Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quiz기능 완성 #59

Merged
merged 24 commits into from
Nov 4, 2024
Merged

quiz기능 완성 #59

merged 24 commits into from
Nov 4, 2024

Conversation

rhehfl
Copy link
Collaborator

@rhehfl rhehfl commented Nov 3, 2024

🔗 관련 이슈

#46

📝작업 내용

quiz 페이지로 들어왔을때 part-id에 대한 퀴즈를 풀 수 있도록 기능을 만들었습니다.
퀴즈 진행도를 백엔드에게 전달하기 위해 api를 기다리느라 PR 올리기까지 조금 지연되었네요..
image

  • 추후 디자인이 나오면 다시 퍼블리싱 할 예정입니다 ㅠㅠ

작업 내용은 크게 다음과 같습니다.

  1. 조합형(combination), 객관식(multipleChoice), OX문제(OXSelector), 단답형(ShortAnswer) 문제 유형별 문제풀이
  2. question폴더에서 UX향상을 위한 빈칸 채우기 로직
  3. 각 문제 결과(정답유무) 백엔드에게 전달
  4. 문제풀이 종료 후 총 결과 공개
  5. useModal 유틸 훅 생성

🔍 변경 사항

  • 라이브러리 html-react-parser, dompurify, query-string 추가

💬리뷰 요구사항 (선택사항)

작업을 더 세분화시켜서 이슈를 생성해야했는데 한번에 quiz에 포함되는 기능을 전부 만드려다보니 file changed가 너무 많이 생긴 것 같습니다.. 갑자기 폭탄을 떨군 것 같네요
요구사항에 관해서는 중간중간 의견을 남겨주셨으면 좋겠는 곳에 추가로 코멘트를 달아놓겠습니다
라이브러리가 여럿 추가되었는데 이에 대한 의견 부탁드립니다.

rhehfl added 21 commits October 14, 2024 01:40
@rhehfl rhehfl added the ✨ Feature 기능 개발 label Nov 3, 2024
@rhehfl rhehfl self-assigned this Nov 3, 2024
Comment on lines +5 to +12
const queryClient = new QueryClient({
defaultOptions: {
queries: {
staleTime: Infinity,
gcTime: Infinity,
},
},
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quiz 기능과는 조금 엇나가는 부분이지만 저희 페이지 특정상 항상 최신화된 데이터가 필요하다고는 생각되지 않아서
항상 캐시된 데이터를 가져올 수 있도록 Provider의 기본 설정을 변경해봤습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluetree7878 혹시 이부분에 대해서 어떻게 생각하시나요?

Comment on lines +1 to +14
import { create } from 'zustand';
import type User from '../types/User';
interface State {
user: Partial<User>;
}
interface Actions {
setUser: (user: User) => void;
}
const useUserStore = create<State & Actions>(set => ({
//나중에 로그인 구현 시 로직 변경하기
user: {},
setUser: user => set(() => ({ user })),
}));
export default useUserStore;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유저의 정보를 저장하기 위해서 Store를 만들었습니다. 코멘트를 쓰다가 생각났는데 생각해보니 어차피 로그인 유지를 위해서는 로컬이나 세션 스토리지를 사용해야 할 것 같네요
나중에 제거하거나 유저의 id or nickName등등의 정보를 쉽게 가져다 쓰는 용도로 사용하면 될 것 같습니다

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

찾아보니 react-cookie라는 라이브러리가 있더라고요, 굳이 백엔드에서 쿠키를 보내줘서 값을 저장하는게 아니라, 프론트영역에서 cookie로 값을 저장할 수 있는 것 같습니다.

react-cookie npm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 리액트에서 로컬스토리지, 세션스토리지 활용하실거면 @modern-kit/react 에 useLocalStorage, useSessionStorage 확인해보세요 :) 직접 사용하셔도 좋고, 가져와서 사용하셔도 좋습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzzRYT @ssi02014 의견 감사합니다. cookie와 로컬스토리지, 세션 스토리지 등 어떤게 더 활용성이 좋을지는 팀원과 조금 더 이야기를 해보고 적용시켜보겠습니다.
특히 오픈소스의 경우 이번에 직접 util함수를 만들어보면서 es-toolkit이나 modern-kit에 들어있는 유틸 함수를 사용하거나 이를 참조하여 직접 구현해보면 좋을 것 같다는 생각이 들었습니다.

Comment on lines 1 to 11
import { useQuery } from '@tanstack/react-query';
import quizzesApis from './../apis/quizzesApis';
const QuizzesQuery = {
get: (params?: { sectionId?: number; partId: number }) => {
return useQuery({
queryKey: ['quizzes', params],
queryFn: () => quizzesApis.getquizzes(params),
});
},
};
export default QuizzesQuery;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/querys 폴더를 추가하여 직접 데이터를 Fetching 해오는 비동기 함수와 그 상태를 관리해주는 query를 분리했습니다

  getquizzes: async (params?: {
    sectionId?: number;
    partId: number;
  }): Promise<Quiz[]> => {
    const response = await api.get('/quizzes', { params });
    return response.data;
  },

분리한 Fetching 함수는 위와 같습니다. params와 return type등이 추가되면서 queryFn이나 mutationFn에서 직접 비동기 함수를 작성했을 때 가독성이 떨어지는 문제도 있는 것 같아 분리한 이유도 있습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

데이터 가져오는 함수는 따로 정의해서 필요한 곳에서 재사용하는 방식으로 코드를 작성하셨네용. 가독성 측면이나, 유지보수면에서 좋을 것 같습니다!

Comment on lines +26 to +57
const options: HTMLReactParserOptions = {
replace: domNode => {
//class명이 empty인(빈칸) html Element를 찾음
if (domNode instanceof Element && domNode.attribs.class === 'empty') {
//그 노드의 id를 가져옴
const id = Number(domNode.attribs.id.match(/\d+$/));
//전역상태(유저의 응답) 배열에서 해당 id의 값 가져와서 빈칸을 TextBlock 컴포넌트로 변경 해당 인덱스가 빈칸이면 그대로 domNod
return userResponseAnswer[id] ? (
<TextBlockButton
onClick={() => spliceUserResponseAnswer(id)}
draggable
onDragStart={() => {
dragOverItem.current = id;
}}
onDragEnter={() => {
dragOverItem.current = id;
}}
onDragEnd={() => {
if (dragItem.current !== null && dragOverItem.current != null) {
swapUserResponseAnswer(dragItem.current, dragOverItem.current);
}
}}
onDragOver={e => e.preventDefault()}
>
{userResponseAnswer[id]}
</TextBlockButton>
) : (
domNode
);
}
},
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 html-react-parser 라이브러리 부분 로직입니다. replace함수는 이 라이브러리가 문자열을 코드로 parse
domNode별로 실행되는 코드입니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라이브러리 관련하여 찾아보니 이거 상당히 괜찮은 라이브러리네요. HTML요소들이 포함된 데이터를 React컴포넌트로 변환하여 쓰는 것이라 하는데, 컴포넌트로 변환하여 쓰기 때문에 XSS 공격도 막아준다네요.

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluetree7878 @rhehfl 정확히는 html-react-parser 만으로는 xss공격에 안전하지 않습니다. DOMPurify와 같은 라이브러리를 함께 사용해주셔야 합니다 :)

https://github.com/remarkablemark/html-react-parser?tab=readme-ov-file#is-this-xss-safe

Copy link
Collaborator Author

@rhehfl rhehfl Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 감사합니다! 어떤 부분에서 안전하지 않은지 조금 더 찾아보았는데요

  if (node.type === 'script' || node.type === 'style') {
      // prevent text in <script> or <style> from being escaped
      // https://facebook.github.io/react/tips/dangerously-set-inner-html.html
      if (node.children[0]) {
        props.dangerouslySetInnerHTML = {
          __html: node.children[0].data
        };
      }

html-react-parse가 내부적으로 사용하는 코드입니다 문자열을 랜더링시키기 위해서 dangerouslySetInnerHTML`를 사용하고 있습니다
이에 관해서 react 공식문서에서는

dangerouslySetInnerHTML은 브라우저 DOM에서 innerHTML을 사용하기 위한 React의 대체 방법입니다. 일반적으로 코드에서 HTML을 설정하는 것은 사이트 간 스크립팅 공격에 쉽게 노출될 수 있기 때문에 위험합니다. 따라서 React에서 직접 HTML을 설정할 수는 있지만, 위험하다는 것을 상기시키기 위해 dangerouslySetInnerHTML을 작성하고 __html 키로 객체를 전달해야 합니다.
말 그대로 위험하기 때문에 확실히DOMPurify를 같이 사용하는게 좋을 것 같습니다.

src/store/useClientQuizStore.ts Outdated Show resolved Hide resolved
Comment on lines 93 to 96
interface TextBlockLiProps {
$selected?: boolean;
}
export const TextBlockButton = styled.button<TextBlockLiProps>`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컴포넌트 명은 button인데, interface명은 Li로 되어있네요 의도한 부분일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에는 li태그로 만들었는데 추후에 button으로 수정하면서 깜빡하고 인터페이스 네임을 못 바꾼 것 같습니다
꼼꼼하게 코드 봐주셔서 감사합니다 ㅠㅠ

Comment on lines +9 to +17
<OXButton
onClick={() => setUserResponseAnswer('O')}
$backGroundColor={userResponseAnswer[0] === 'O'}
/>
<CharacterBox $margin="0px 102px">캐릭터 들어갈예정</CharacterBox>
<OXButton
onClick={() => setUserResponseAnswer('X')}
$backGroundColor={userResponseAnswer[0] === 'X'}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onClick함수가 중복됩니다 하나의 함수로 만들어서 O인지 X인지에 따라서 값을 주면 좋을 것 같다는 생각입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onClick 이벤트가 발생했을때 각 버튼에서 setUserResponseAnswer라는 함수로 답을 O,X로 지정해주고 있습니다
$backGroundColor의 값으로 boolean값을 전달시켜서 유저가 클릭한 버튼의 색을 변경시켜주는 역할을 하고 있는데
어떤 부분을 더 함수로 만들어야 하는지 추가 의견을 달아주실 수 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 제가 잘못 봤네요. onClick이벤트 안에서 모든 로직을 처리하고 있는 줄 알았습니다.

여기서 굳이 나눈다면, Component로 쪼개서, props로 전달하는 방법이 있을 것 같은데, 이건 오버엔지니어링이라는 생각이 드네요.

좀 더 꼼꼼히 봤어야 했는데, 혼동을 드려 죄송합니다😥

Comment on lines 25 to 27
<button type="button" onClick={goToLearnPage}>
learn페이지로 돌아가기
</button>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가 로직이 들어가는 부분일까요? 단순히 페이지를 넘기는 목적이라면 Link 컴포넌트 사용도 고려해 보면 좋을 것 같습니다.

이 부분은 선호도 차이일 것 같긴 하네요.
제 생각은 dom요소에 a태그가 들어가면 페이지를 넘기는 목적을 확실하게 알 수 있으니깐 좋을 것 같다는 의견입니다😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새벽에 급급하게 페이지를 넘기는 부분을 만들다보니 생각없이 버튼에 onClick 이벤트를 달아야 한다는 생각으로 위와같은 로직을 만들었네요 저도 @zzzRYT님 의견대로 Link 컴포넌트를 사용해서 페이지 이동을 시키는게 더 좋을 것 같습니다

const emptyChangeToDiv = (text: string) => {
let index = 0;
const newText = text.replace(/(#empty#)/g, () => {
const replacement = `<div id="emptyBlock${index}" class = "empty"></div>`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 궁금한 내용인데, class를 활용해서 css를 작성한 이유가 있을까요?

styled-component를 활용하면 dom생성이 불가한 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 그렇습니다 styled-component를 활용하면 의 형태로 작성하게 되는데요 이는 실제 JSX 문서에서 사용하는거와는 다르게 <div class="해시값'>형태로 변환되지 않고 크롬의 개발자 도구로 확인해보면라는 이상한 형태로 랜더링 되게 됩니다. 당연하게도 html 태그의 역할과 스타일링 둘다 불가능하기 때문에

  1. 일반 css를 사용하기
  2. html-react-parse의 replace 함수를 통해서 추가로 styled-component의요소로 변경시키는 방법이 있습니다
    저는 둘중에 조금 더 로직이 간단해지는 1번째 방법을 선택했습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzzRYT @rhehfl

https://stackoverflow.com/questions/44643424/how-to-parse-html-to-react-component

https://github.com/remarkablemark/html-react-parser?tab=readme-ov-file#dont-change-case-of-tags

처음에는 html-react-parser가 일반 태그들만 가능 할 것으로 생각했는데 위 내용들을 보면 컴포넌트도 가능해보이네요 :) 테스트는 필요해보입니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import React from 'react';
import { render } from 'react-dom';
import parse from 'html-react-parser';
import DOMPurify from 'dompurify';

const SpecialButton = ({ children, color }) => (
  <button style={{color}}>{children}</button>
);

const htmlFromCMS = `
<div>Hi, 
  <SpecialButton color="red">My Button</SpecialButton>
</div>`;

const htmlFrom = (htmlString) => {
        const cleanHtmlString = DOMPurify.sanitize(htmlString,
          { USE_PROFILES: { html: true } });
        const html = parse(cleanHtmlString);
        return html;
}

const App = () => (
  <div>
     {htmlFromCMS && htmlFrom(htmlFromCMS)}
  </div>
);


render(<App />, document.getElementById('root'));

제가 테스트 해봤을때는 랜더링이 안되었는데 이 글을 보니 html-react-parser로 컴포넌트도 되는 것 같아 보이네요
레퍼런스 제공 감사합니다!

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl lowerCaseTags 옵션까지 확인해보세요 저도 테스트는 안해봐서 한번되는지 테스트해보시고 알려주세요 :) ㅎㅎㅎㅎ
https://github.com/remarkablemark/html-react-parser?tab=readme-ov-file#dont-change-case-of-tags

Copy link
Collaborator Author

@rhehfl rhehfl Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import { renderToString } from 'react-dom/server';

const SpecialButton = ({
    children,
    color,
  }: PropsWithChildren<{ color: string }>) => (
    <button style={{ color }}>{children}</button>
  );
//<QuestionSection/>은 스타일드 컴포넌트 요소
  const renderButton = renderToString(
    <QuestionSection>
      <SpecialButton color="red">MyButton!!!</SpecialButton>
    </QuestionSection>
  );
  const option = {
    htmlparser2: {
      lowerCaseTags: false,
    },
  };
  return parse(renderButton, option);

renderToString 메소드를 통해서 컴포넌트를 랜더링 시킬 수 있는 것 같습니다! 이는 리액트의 고유 속성을 포함한 문자열로 변경시켜주는 메소드인데요
콘솔을 찍어보면 아래와같이 styled-component도 내부적으로 잘 변환시켜 주는 것 같습니다!

<section class="styles__QuestionSection-nfDVw gzZqgo"><button style="color:red">MyButton!!!</button></section>

이를 html-react-parser랑 조합해서 사용자 지정 컴포넌트도 랜더링 시킬 수 있는 것 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 👍👍👍👍

Comment on lines 23 to 29
if (
//답 수랑 내가 선택한 답 (공백빼고) 갯수 비교 정답보다 선택한게 많으면 안되니
answer.length > userResponseAnswer.filter(item => item).length
) {
pushUserResponseAnswer(choice);
}
}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로직이 좀 더 커지면, 함수로 나눠서 사용하는게 좋지 않을까 생각합니다🧐

PR을 보면서 느낀건데, 대부분의 click eventonClick 속성 내부에 들어가 있던 것 같습니다. 간단한 로직은 사용해도 상관없을 것 같지만, 로직이 조금만 커져도 return부에 있는 태그들과 섞이면 가독성이 떨어진다는게 저의 의견입니다😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 로직들을 만들때마다 항상 어떤 기준으로 함수로 분류해야 할 지가 고민이네요..
이러한 함수를 구현할 때 보면 내부 로직에서도 더 세분화 시킬 수 있는 로직이 생기더라구요.. 이런 util함수들을 직접 다 구현을 하기가 시간적으로 불가능하다보니 오픈소스를 사용하는걸 생각 해 봐야겠습니다..

Comment on lines 30 to 33
const [partId] = useQueryParams(['part-id']);
if (partId === null) {
return <div>404</div>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분과는 다른 내용이긴하지만, 404페이지 만들면서 해당 return부에도 컴포넌트를 넣어주면 될 것 같네요.

찾아보니깐 react에서는 404페이지에 대한 처리를 Route부분에서 *를 사용해서 한다고 하네요.

참고 블로그
해당 블로그를 참고해 보면
중첩 라우팅을 통해서 이미 존재하는 라우트 경로 뒤에 동적으로 붙는 부분도 라우팅을 시킬 수 있는 것 같네요

위 의견을 적용시키면, 해당 로직을 지워도 될 것 같다는 생각이 들긴하네요😶

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzzRYT 좋네요 :) 404페이지를 만들고 react-router-dom 활용해서 리다이렉트 시켜도 좋을 것 같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의견 감사합니다! 저희 미니 프로젝트에서도 사용하지 않는 페이지에 대해서는 *를 사용해서 404페이지로 리다이렉트를 시켰는데요 아마 이번에도 404페이지가 만들어지면 같은 로직을 사용 할 것 같습니다!

Comment on lines 54 to 61
const componentChoice = componentMapping<
Pick<Quiz, 'answerChoice'> | Pick<Quiz, 'answer'>
>({
COMBINATION: Combination,
MULTIPLE_CHOICE: MultipleChoice,
OX_SELECTOR: OXSelector,
SHORT_ANSWER: ShortAnswer,
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

componentChoice는 저희가 개발하면서 사용하는 Component와 혼동될 가능성이 있고, 요소선택은 다소 두루뭉실한 경향이 있는 것 같습니다.

questionTypeChoice와 같이, 문제의 형식을 선택할 수 있다는 네이밍이 컨벤션을 벗어나지 않는다면 고려해 보시면 좋을 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl @zzzRYT getComponentByChoiceType 과 같은 네이밍은 어떨까요??
내가 하고자하는 동작을 동사(가져오다, get)로 표기해주면 좋을 것 같습니다 😂
또는 렌더링이라는 의미로 render 도 좋겠네요

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ssi02014 getComponentMappingByChoiceType 은 별로인가요? 어떤 util에서 가져오는지 명확하게 하는게 좋을 것 같다는 생각입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컴포넌트를 맵핑 시켜놓은 객체에서 하나를 고르다 라는 의미에서 componentChoice 라는 함수 네이밍을 했는데요
의미가 제대로 전달되지 못한 것 같습니다.
@zzzRYT 님 의견대로 questionTypeChoice도 좋지만 처음 이 유틸함수를 만들 때 문제 뿐 만 아니라 다른 곳에서도 사용되었으면 좋겠다는 생각이 있어서 question을 붙포넌트를 맵핑 시켜놓은 객체에서 하나를 고르다 라는 의미에서 componentChoice 라는 함수 네이밍을 했는데요
@zzzRYT님과 @ssi02014 님 의견대로 문제 형식에 따라서 하나를 고르다 라는 의미를 담은 네이밍을 고려해 보는것이 좋을 것 같습니다..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzzRYT @rhehfl ㅎㅎㅎ 어떤게 정확한 네이밍이 될 지는 아직 제가 히스토리가 부족하다보니.. 한번 네이밍에 대한 고민이 되면 좋겠습니다 :)

Comment on lines +1 to +14
import { create } from 'zustand';
import type User from '../types/User';
interface State {
user: Partial<User>;
}
interface Actions {
setUser: (user: User) => void;
}
const useUserStore = create<State & Actions>(set => ({
//나중에 로그인 구현 시 로직 변경하기
user: {},
setUser: user => set(() => ({ user })),
}));
export default useUserStore;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

찾아보니 react-cookie라는 라이브러리가 있더라고요, 굳이 백엔드에서 쿠키를 보내줘서 값을 저장하는게 아니라, 프론트영역에서 cookie로 값을 저장할 수 있는 것 같습니다.

react-cookie npm

Comment on lines 2 to 12
const usersApis = {
putQuizzesProgress: ({
userId,
quizId,
body,
}: {
userId: number;
quizId: number;
body: Record<'isCorrect', boolean>;
}) => api.put(`/users/${userId}/progress/quizzes/${quizId}`, body),
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 반환 값이 없을까요? 없다면 Promise<void> 타입으로 해당 api의 반환타입을 정의해볼 수 있을 것 같아요 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반환값을 사용하지 않기 때문에 굳이 필요없다고 생각해서 지정을 안해줬었습니다
근데 @ssi02014 님 말대로 반환값이 없을 때 Promise<void>로 지정해놓는게 mutation에서도 타입추론이 되고 더 좋은 것 같습니다.
의견 감사합니다!

@@ -0,0 +1,46 @@
import progressQuery from '../../../querys/usersQuery';
Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl ㅎㅎㅎ 절대경로를 설정하면 좋을 것 같아요~ 모듈을 가져 올 때 간단해집니다.
아 그리고 querys 가 아니고 queries 여야 합니다 😂

// as-is
import progressQuery from '../../../querys/usersQuery';

// to-be
import progressQuery from '@/querys/usersQuery';

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl npm vite-tsconfig-paths으로 vite 환경에서 쉽게 경로 설정이 가능하네요 참고하시면 좋을 것 같습니다.

Copy link
Collaborator Author

@rhehfl rhehfl Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아이고.. 앞으로 영어 네이밍은 무조건 Ai & 번역기 사용해서 해야겠습니다.. 부끄럽네요
@ssi02014 @zzzRYT 경로 관련 레퍼런스 감사합니다 @를 통해서 절대경로로 지정 할 수 있다는것은 처음 알았네요

result.push(queryParams.get(param));
});
return result;
}
Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 이전에 제가 테스트했을 때 URLSearchParams가 생각보다 성능이 떨어지더라구여 ㅎㅎㅎ 이미 query-string 라이브러리를 설치하셨으니 queryString.parse를 활용해도 좋을 것 같아요 :)

아래는 단순 예제입니다.

const getParams = (params: string[]): string[] => {
  const parsedQuery = queryString.parse(location.search); 
  // 이거 그냥 window.location 써도 되지 않나요? ㅎㅎ 
  // 이러면 useLocation을 안써도 되니 굳이 훅으로 관리하지 않아도 됩니다.
  const result: string[] = [];

  params.forEach((param) => {
    result.push(parsedQuery[param] as string);
  });
  return result;
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query-string라이브러리로 파싱이 가능하군요 변명이긴 하지만 메인 프로젝트를 하면서 도입한 라이브러리가 많다보니
각 라이브러리에 대한 깊이가 많이 부족한걸 느끼네요
예제까지 정말 감사합니다!

Comment on lines 6 to 21
const useRefreshWaringAlert = () => {
useEffect(() => {
const handleBeforeUnload = (event: BeforeUnloadEvent) => {
// 경고 메시지를 설정합니다.
event.preventDefault();
event.returnValue = ''; // 대부분의 브라우저에서 필수로 설정해야 작동
};

window.addEventListener('beforeunload', handleBeforeUnload);

// 컴포넌트가 언마운트될 때 이벤트 리스너를 제거합니다.
return () => {
window.removeEventListener('beforeunload', handleBeforeUnload);
};
}, []);
};
Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 좋은데요? ㅎㅎㅎ 👍 modern-kit에도 추가해도 되겠네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다! 바로 modern-kit으로 달려가겠습니다

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 다만, 네이밍을 약간 제안드리자면 refresh, 창 닫기 모두 befoureUnload 이벤트가 호출되다보니 Refresh로 한정하는 네이밍보다는 좀 더 범용적인 네이밍이 좋을 것 같아요.
useBeforeUnload 이 괜찮아 보이네요 ㅎㅎㅎ 실제로 아래와 같이 useBeforeUnload 이름의 레퍼런스가 많이 존재합니다

Reference

https://github.com/streamich/react-use/blob/master/docs/useBeforeUnload.md
https://reactrouter.com/en/main/hooks/use-before-unload

interface

function useBeforeUnload({ enabled, beforeUnloadAction }: {
    enabled: boolean;
    beforeUnloadAction: (event: BeforeUnloadEvent) => void;
}): void

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl beforeunload 이벤트를 바인딩 할 옵션 enabled와 이벤트 바인딩 후 특정 액션 함수를 호출하게끔 beforUnloadAction이 추가되면 좋을 것 같네요

interface UseBeforeUnloadProps {
  enabled?: boolean;
  beforeUnloadAction?: (event: BeforeUnloadEvent) => void;
}

export function useBeforeUnload({
  enabled = true, // 기본적으로 이벤트 바인딩
  beforeUnloadAction, // noop으로 기본 함수 설정도 가능
}: UseBeforeUnloadProps = {}): void {
  // usePreservedCallback는 함수의 참조를 유지해주는 커스텀 훅입니다. @modern-kit 참고
  // props로 전달하는 함수의 참조를 유지해야되는 이유는 함수도 결국 객체이기 때문에  props로 전달하는 함수는 리렌더링마다 재생성되기 때문에 
  // useCallback으로 관리하더라도 매번 재생성됩니다. 이런 문제를 해결하기 위해 usePreservedCallback와 같은 훅을 사용
  const handleBeforeUnload = usePreservedCallback(
    (event: BeforeUnloadEvent) => {
      event.preventDefault();
      
      // beforeUnloadAction이 있을 경우 호출
      if (beforeUnloadAction) {
        beforeUnloadAction(event);
      }
      return (event.returnValue = ""); // MDN 보니까 return 해주던데 return문을 추가했습니다.
     // https://developer.mozilla.org/ko/docs/Web/API/Window/beforeunload_event#%EC%98%88%EC%A0%9C
    }
  );

  useEffect(() => {
    if (!enabled) return; // enabled가 false이면 이벤트 바인딩하지 않음

    window.addEventListener("beforeunload", handleBeforeUnload);

    return () => {
      window.removeEventListener("beforeunload", handleBeforeUnload);
    };
  }, [enabled, handleBeforeUnload]);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음.. 네이밍이 어긋난 부분이 생각보다 많네요 @ssi02014 님 말대로 refresh이벤트에만 반응하는게 아니다보니 useBeforeUnload 가 더 적절해 보이네요
레퍼런스까지 제공해주시고 너무 감사합니다

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 7 to 23
const useBeforeUnload = ({
enabled = true, // 기본적으로 이벤트 바인딩
}: UseBeforeUnloadProps = {}): void => {
const handleBeforeUnload = (event: BeforeUnloadEvent) => {
event.preventDefault();
return (event.returnValue = '');
};
useEffect(() => {
if (!enabled) return; // enabled가 false이면 이벤트 바인딩하지 않음

window.addEventListener('beforeunload', handleBeforeUnload);

return () => {
window.removeEventListener('beforeunload', handleBeforeUnload);
};
}, [enabled]);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 beforeUnloadAction을 활용하려고 handleBeforeUnload를 useEffect 외부로 뺐는데요.
이런 경우에는 그냥 useEffect 내에 정의하는게 좋을 것 같네요

  useEffect(() => {
    if (!enabled) return; // enabled가 false이면 이벤트 바인딩하지 않음

    const handleBeforeUnload = (event: BeforeUnloadEvent) => {
      event.preventDefault();
      return (event.returnValue = '');
    };

    window.addEventListener('beforeunload', handleBeforeUnload);

    return () => {
      window.removeEventListener('beforeunload', handleBeforeUnload);
    };
  }, [enabled]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 확실히 외부에서 이벤트를 받는게 아니라면 그냥 useEffect 내부에 두는게 더 좋을 것 같습니다.
처음에는 @ssi02014님이 제공해주신 beforeunload 함수를 보고 처음에 action을 추가적으로 받지 않아도 괜찮지 않을까 라는 생각이 들어서 굳이 넣지 않은 부분인데요
다시한번 생각해보니 추후에 사용자가 떠날 때 진행 상황을 추가로 저장한다던지 아얘 다른 페이지로 리다이렉트 시켜버리는 로직을 추가할 수 있을 것 같다는 생각에 다시 action을 추가로 받는게 확장성 측면에서 더 좋은 hook 이라고 생각되네요

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 좋아요 ~ 제가 최초 제안한 useBeforeUnload 훅은 불특정 다수가 범용적으로 사용 할 수 있게 설계를 했던 부분입니다. (modern-kit에 추가하려고 했기 때문)
action의 사용 여부는 그냥 사용자가 필요할 때 직접! 결정할 수 있기 때문이죠 ㅎㅎ (옵셔널 옵션)

Comment on lines 14 to 16
const arraysEqual = <T>(array1: T[], array2: T[]) => {
return JSON.stringify(array1) === JSON.stringify(array2);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON.stringify 변환 후 비교는 성능상 생각보다 느려요.
단순 비교 원시값 비교만 한다면 반복문과 includes로 비교해도 좋고, 객체/배열/중첩 객체/중첩 배열 등 다양한 케이스에 대응하려면 재귀적으로 구현하는게 성능상 좋습니다.

@modern-kit의 isEqual을 참고해보세용~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@modern-kit 의 array쪽에서 찾아봤을땐 배열 비교하는쪽이 없어서modern-kit 의 array쪽에서 찾아봤을땐 배열 비교하는쪽이 없어서 기존 로직을 유지했는데 validator 쪽에 있었군요 감사합니다!

Comment on lines +18 to +28
const compact = <T>(arr: T[] | readonly T[]): Retained<T>[] => {
const result: Retained<T>[] = [];

for (const item of arr) {
if (item) {
result.push(item as Retained<T>);
}
}

return result;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 13 to 20
const isArrayContentEqual = (array1: any[], array2: any[]): boolean => {
if (array1.length !== array2.length) return false;
for (let i = 0; i < array1.length; i++) {
if (array1[i] !== array2[i]) {
return false;
}
}
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하는 곳에서는 1차원 배열의 요소값에 대해서 순서와 값이 일치하는지만 확인하면 될 것 같아서 큰 로직 없이 간단하게 만들어 봤습니다!
함수 이름도 기존 arrayEqual에서 내부 값만 비교한다는 뜻인 isArrayContentEqual로 변경했습니다.

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isArrayContentEqual = (array1: any[], array2: any[]): boolean => {
if (array1.length !== array2.length) return false;
for (let i = 0; i < array1.length; i++) {
if (array1[i] !== array2[i]) {
return false;
}
}
return true;
const isEqualArray = <T>(array1: T[] | readonly T[], array2: T[] | readonly T[]): boolean => {
if (array1.length !== array2.length) return false;
for (let i = 0; i < array1.length; i++) {
if (array1[i] !== array2[i]) {
return false;
}
}
return true;
}

@rhehfl 좋아요 프로젝트 상황에 맞게 스펙을 가져가면 됩니다 🤗

몇 가지 의견 드려볼게요 isEqualArray 네이밍은 어떨까요? 좀 더 단순하지만 목적을 충분히 전달합니다.

제네릭 타입(T)을 활용하면 좋을 것 같아요 :) any 타입보다 더욱 명확합니다.

readonly를 추가 한 이유는 as const 배열에도 호환하기 위함입니다.

array1와 array2에 같은 제네릭 T를 넣은 이유는 타입 추론 시에 자동으로 둘을 확장한 타입으로 추론됩니다 (아래 as const 예제 참고)

// number[] 사용
isEqualArray([1, 2, 3], [3, 4, 5]);

// 추론되는 타입
const isEqualArray: <number>(array1: number[] | readonly number[], array2: number[] | readonly number[]) => boolean
// as const 사용
isEqualArray([1, 2, 3] as const, [3, 4, 5] as const);

// 추론되는 타입
const isEqualArray: <5 | 1 | 2 | 3 | 4>(array1: (5 | 1 | 2 | 3 | 4)[] | readonly (5 | 1 | 2 | 3 | 4)[], array2: (5 | 1 | 2 | 3 | 4)[] | readonly (5 | 1 | 2 | 3 | 4)[]) => boolean

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

내부 요소를 비교한다는 의미에서 isArrayContentEqual라는 함수 네임을 붙였는데 @ssi02014 님 말씀대로 isEqualArray가 조금 더 심플하면서 비슷한 목적을 전달하는 것 같네요 내부 요소 비교 라는 추가 설명은 어차피 주석에 달려있으니 같은 의도가 전달된다면 심플한 쪽을 선택하는게 맞는 것 같습니다!
제네릭 타입 같은 경우에는 초기에 함수를 만들었을 때 사용했습니다.

const arraysEqual = <T>(array1: T[], array2: T[]) => {
  return JSON.stringify(array1) === JSON.stringify(array2);
};

하지만 @example쪽 예시처럼 isEqualArray([1,2,3,4,5],['a','',3,1,true])//false
와 같이 배열 내부에 다양한 타입이 들어올 수 있다고 생각해서 any[]타입으로 변경했습니다
이 부분에 관해서는 @ssi02014님 의견이 궁금합니다부 요소를 비교한다는 의미에서 isArrayContentEqual라는 함수 네임을 붙였는데 @ssi02014 님 말씀대로 isEqualArray가 조금 더 심플하면서 비슷한 목적을 전달하는 것 같네요 내부 요소 비교 라는 추가 설명은 어차피 주석에 달려있으니 같은 의도가 전달된다면 심플한 쪽을 선택하는게 맞는 것 같습니다!
제네릭 타입 같은 경우에는 초기에 함수를 만들었을 때 사용했습니다.

const arraysEqual = <T>(array1: T[], array2: T[]) => {
  return JSON.stringify(array1) === JSON.stringify(array2);
};

하지만 @example쪽 예시처럼 isEqualArray([1,2,3,4,5],['a','',3,1,true])//false
와 같이 배열 내부에 다양한 타입이 들어올 수 있다고 생각해서 any[]타입으로 변경했습니다.
타입이 다른 경우에는 에초에 false 값이니 제네릭 T로 array의 타입을 받을지 그럼에도 함수 자체의 목적은 두 배열의 요소가 같은지 아닌지 비교하는것이니 any[]타입으로 둘지 고민입니다.

array1: number[] | readonly number[], array2: number[] | readonly number[] 인자를 이와같이 배열이 as const인 경우도 생각해서 유니온 타입으로 받는건 정말 좋은 것 같습니다!

Copy link
Collaborator

@ssi02014 ssi02014 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 이런 경우에는 함수의 사용케이스를 고민해보면 좋을 것 같아요!

일반적으로 isEqualArray 함수를 통해 두 배열의 값을 비교 할 때 유사한 타입의 배열을 비교 할 경우가 많지 완전히 동떨어진 타입의 두 배열을 비교하는 케이스는 적을 것 같은데요! 🤔

물론, 예시로 주신 isEqualArray([1, 2, 3, 4, 5],['a', '', 3, 1, true]); //false 케이스는 해당 함수가 제대로 동작해요. 왜냐하면 array1/array2의 타입이 (string | number | boolean)[]로 타입이 확장되기 때문입니다.

isEqualArray([1, 2, 3], [1, 2, 4]); // 동일한 타입의 두 배열
isEqualArray([1, 2, 3], [1, 2, '4']); // 2번째 배열에 문자열이 하나 있기 때문에 (string | number)[] 로 확장 될 것이기 때문에 문제 없음
isEqualArray([1, 2, 3], [1, 2, '4', true]);  //  (string | number | boolean)[]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhehfl 타입이 완전히 호환되지 않고, 동떨어진 두 배열의 비교 케이스가 적을 것으로 예상이되고, 특별한 경우가 아니라면 타입 확장으로 잘 동작할 것으로 예상되기 때문에 any보다는 제네릭이 낫지 않을까가 제 의견입니다 ㅎㅎㅎ 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아!
어차피 제네릭으로 두어도 (string | number | boolean)[]처럼 타입 확장이 되니 제네릭으로 두는게 더 좋을 것 같네요
확실히 any보다는 제네릭이 좋을 듯 합니다.
리뷰 감사합니다!

Comment on lines 13 to 19
const isEqualArray = <T>(
array1: T[] | readonly T[],
array2: T[] | readonly T[]
): boolean => {
if (array1.length !== array2.length) return false;
for (let i = 0; i < array1.length; i++) {
if (array1[i] !== array2[i]) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수 이름 기존 isArrayContentEqual 에서 isEqualArray으로 수정하고
타입도 any[]에서 array1: T[] | readonly T[], array2: T[] | readonly T[]로 수정했습니다.

@rhehfl rhehfl merged commit 7555211 into develop Nov 4, 2024
@rhehfl rhehfl deleted the feature/#46/quiz branch November 4, 2024 15:35
@rhehfl rhehfl mentioned this pull request Nov 20, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants