-
Notifications
You must be signed in to change notification settings - Fork 0
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
quiz기능 완성 #59
Conversation
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
staleTime: Infinity, | ||
gcTime: Infinity, | ||
}, | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quiz 기능과는 조금 엇나가는 부분이지만 저희 페이지 특정상 항상 최신화된 데이터가 필요하다고는 생각되지 않아서
항상 캐시된 데이터를 가져올 수 있도록 Provider
의 기본 설정을 변경해봤습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetree7878 혹시 이부분에 대해서 어떻게 생각하시나요?
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유저의 정보를 저장하기 위해서 Store를 만들었습니다. 코멘트를 쓰다가 생각났는데 생각해보니 어차피 로그인 유지를 위해서는 로컬이나 세션 스토리지를 사용해야 할 것 같네요
나중에 제거하거나 유저의 id or nickName등등의 정보를 쉽게 가져다 쓰는 용도로 사용하면 될 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찾아보니 react-cookie
라는 라이브러리가 있더라고요, 굳이 백엔드에서 쿠키를 보내줘서 값을 저장하는게 아니라, 프론트영역에서 cookie
로 값을 저장할 수 있는 것 같습니다.
There was a problem hiding this comment.
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
확인해보세요 :) 직접 사용하셔도 좋고, 가져와서 사용하셔도 좋습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/querys/quizzesQuery.ts
Outdated
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; |
There was a problem hiding this comment.
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
에서 직접 비동기 함수를 작성했을 때 가독성이 떨어지는 문제도 있는 것 같아 분리한 이유도 있습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
데이터 가져오는 함수는 따로 정의해서 필요한 곳에서 재사용하는 방식으로 코드를 작성하셨네용. 가독성 측면이나, 유지보수면에서 좋을 것 같습니다!
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 | ||
); | ||
} | ||
}, | ||
}; |
There was a problem hiding this comment.
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
별로 실행되는 코드입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
라이브러리 관련하여 찾아보니 이거 상당히 괜찮은 라이브러리네요. HTML요소들이 포함된 데이터를 React컴포넌트로 변환하여 쓰는 것이라 하는데, 컴포넌트로 변환하여 쓰기 때문에 XSS 공격도 막아준다네요.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/features/quiz/styles.ts
Outdated
interface TextBlockLiProps { | ||
$selected?: boolean; | ||
} | ||
export const TextBlockButton = styled.button<TextBlockLiProps>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컴포넌트 명은 button인데, interface명은 Li로 되어있네요 의도한 부분일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에는 li
태그로 만들었는데 추후에 button
으로 수정하면서 깜빡하고 인터페이스 네임을 못 바꾼 것 같습니다
꼼꼼하게 코드 봐주셔서 감사합니다 ㅠㅠ
<OXButton | ||
onClick={() => setUserResponseAnswer('O')} | ||
$backGroundColor={userResponseAnswer[0] === 'O'} | ||
/> | ||
<CharacterBox $margin="0px 102px">캐릭터 들어갈예정</CharacterBox> | ||
<OXButton | ||
onClick={() => setUserResponseAnswer('X')} | ||
$backGroundColor={userResponseAnswer[0] === 'X'} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onClick함수가 중복됩니다 하나의 함수로 만들어서 O인지 X인지에 따라서 값을 주면 좋을 것 같다는 생각입니다.
There was a problem hiding this comment.
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
값을 전달시켜서 유저가 클릭한 버튼의 색을 변경시켜주는 역할을 하고 있는데
어떤 부분을 더 함수로 만들어야 하는지 추가 의견을 달아주실 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 제가 잘못 봤네요. onClick이벤트 안에서 모든 로직을 처리하고 있는 줄 알았습니다.
여기서 굳이 나눈다면, Component로 쪼개서, props로 전달하는 방법이 있을 것 같은데, 이건 오버엔지니어링이라는 생각이 드네요.
좀 더 꼼꼼히 봤어야 했는데, 혼동을 드려 죄송합니다😥
<button type="button" onClick={goToLearnPage}> | ||
learn페이지로 돌아가기 | ||
</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가 로직이 들어가는 부분일까요? 단순히 페이지를 넘기는 목적이라면 Link 컴포넌트 사용도 고려해 보면 좋을 것 같습니다.
이 부분은 선호도 차이일 것 같긴 하네요.
제 생각은 dom요소에 a태그가 들어가면 페이지를 넘기는 목적을 확실하게 알 수 있으니깐 좋을 것 같다는 의견입니다😊
There was a problem hiding this comment.
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>`; |
There was a problem hiding this comment.
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생성이 불가한 건가요?
There was a problem hiding this comment.
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 태그의 역할과 스타일링 둘다 불가능하기 때문에
- 일반 css를 사용하기
html-react-parse
의 replace 함수를 통해서 추가로styled-component
의요소로 변경시키는 방법이 있습니다
저는 둘중에 조금 더 로직이 간단해지는 1번째 방법을 선택했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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가 일반 태그들만 가능 할 것으로 생각했는데 위 내용들을 보면 컴포넌트도 가능해보이네요 :) 테스트는 필요해보입니다
There was a problem hiding this comment.
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
로 컴포넌트도 되는 것 같아 보이네요
레퍼런스 제공 감사합니다!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
랑 조합해서 사용자 지정 컴포넌트도 랜더링 시킬 수 있는 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhehfl 👍👍👍👍
src/features/quiz/ui/Combination.tsx
Outdated
if ( | ||
//답 수랑 내가 선택한 답 (공백빼고) 갯수 비교 정답보다 선택한게 많으면 안되니 | ||
answer.length > userResponseAnswer.filter(item => item).length | ||
) { | ||
pushUserResponseAnswer(choice); | ||
} | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로직이 좀 더 커지면, 함수로 나눠서 사용하는게 좋지 않을까 생각합니다🧐
PR을 보면서 느낀건데, 대부분의 click event
가 onClick
속성 내부에 들어가 있던 것 같습니다. 간단한 로직은 사용해도 상관없을 것 같지만, 로직이 조금만 커져도 return
부에 있는 태그
들과 섞이면 가독성이 떨어진다는게 저의 의견입니다😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 로직들을 만들때마다 항상 어떤 기준으로 함수로 분류해야 할 지가 고민이네요..
이러한 함수를 구현할 때 보면 내부 로직에서도 더 세분화 시킬 수 있는 로직이 생기더라구요.. 이런 util함수들을 직접 다 구현을 하기가 시간적으로 불가능하다보니 오픈소스를 사용하는걸 생각 해 봐야겠습니다..
src/pages/quiz/Quiz.tsx
Outdated
const [partId] = useQueryParams(['part-id']); | ||
if (partId === null) { | ||
return <div>404</div>; | ||
} |
There was a problem hiding this comment.
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부분에서 *
를 사용해서 한다고 하네요.
참고 블로그
해당 블로그를 참고해 보면
중첩 라우팅
을 통해서 이미 존재하는 라우트 경로 뒤에 동적으로 붙는 부분도 라우팅을 시킬 수 있는 것 같네요
위 의견을 적용시키면, 해당 로직을 지워도 될 것 같다는 생각이 들긴하네요😶
There was a problem hiding this comment.
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 활용해서 리다이렉트 시켜도 좋을 것 같아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의견 감사합니다! 저희 미니 프로젝트에서도 사용하지 않는 페이지에 대해서는 *
를 사용해서 404
페이지로 리다이렉트를 시켰는데요 아마 이번에도 404
페이지가 만들어지면 같은 로직을 사용 할 것 같습니다!
src/pages/quiz/Quiz.tsx
Outdated
const componentChoice = componentMapping< | ||
Pick<Quiz, 'answerChoice'> | Pick<Quiz, 'answer'> | ||
>({ | ||
COMBINATION: Combination, | ||
MULTIPLE_CHOICE: MultipleChoice, | ||
OX_SELECTOR: OXSelector, | ||
SHORT_ANSWER: ShortAnswer, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
componentChoice
는 저희가 개발하면서 사용하는 Component
와 혼동될 가능성이 있고, 요소선택
은 다소 두루뭉실한 경향이 있는 것 같습니다.
questionTypeChoice
와 같이, 문제의 형식
을 선택할 수 있다는 네이밍이 컨벤션을 벗어나지 않는다면 고려해 보시면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssi02014 getComponentMappingByChoiceType 은 별로인가요? 어떤 util에서 가져오는지 명확하게 하는게 좋을 것 같다는 생각입니다.
There was a problem hiding this comment.
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 님 의견대로 문제 형식에 따라서 하나를 고르다 라는 의미를 담은 네이밍을 고려해 보는것이 좋을 것 같습니다..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찾아보니 react-cookie
라는 라이브러리가 있더라고요, 굳이 백엔드에서 쿠키를 보내줘서 값을 저장하는게 아니라, 프론트영역에서 cookie
로 값을 저장할 수 있는 것 같습니다.
src/apis/usersApis.ts
Outdated
const usersApis = { | ||
putQuizzesProgress: ({ | ||
userId, | ||
quizId, | ||
body, | ||
}: { | ||
userId: number; | ||
quizId: number; | ||
body: Record<'isCorrect', boolean>; | ||
}) => api.put(`/users/${userId}/progress/quizzes/${quizId}`, body), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhehfl 반환 값이 없을까요? 없다면 Promise<void>
타입으로 해당 api의 반환타입을 정의해볼 수 있을 것 같아요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반환값을 사용하지 않기 때문에 굳이 필요없다고 생각해서 지정을 안해줬었습니다
근데 @ssi02014 님 말대로 반환값이 없을 때 Promise<void>
로 지정해놓는게 mutation
에서도 타입추론이 되고 더 좋은 것 같습니다.
의견 감사합니다!
src/features/quiz/ui/ResultModal.tsx
Outdated
@@ -0,0 +1,46 @@ | |||
import progressQuery from '../../../querys/usersQuery'; |
There was a problem hiding this comment.
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';
There was a problem hiding this comment.
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 환경에서 쉽게 경로 설정이 가능하네요 참고하시면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/hooks/useQueryParams.ts
Outdated
result.push(queryParams.get(param)); | ||
}); | ||
return result; | ||
} |
There was a problem hiding this comment.
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;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query-string
라이브러리로 파싱이 가능하군요 변명이긴 하지만 메인 프로젝트를 하면서 도입한 라이브러리가 많다보니
각 라이브러리에 대한 깊이가 많이 부족한걸 느끼네요
예제까지 정말 감사합니다!
src/hooks/useRefreshWaringAlert.ts
Outdated
const useRefreshWaringAlert = () => { | ||
useEffect(() => { | ||
const handleBeforeUnload = (event: BeforeUnloadEvent) => { | ||
// 경고 메시지를 설정합니다. | ||
event.preventDefault(); | ||
event.returnValue = ''; // 대부분의 브라우저에서 필수로 설정해야 작동 | ||
}; | ||
|
||
window.addEventListener('beforeunload', handleBeforeUnload); | ||
|
||
// 컴포넌트가 언마운트될 때 이벤트 리스너를 제거합니다. | ||
return () => { | ||
window.removeEventListener('beforeunload', handleBeforeUnload); | ||
}; | ||
}, []); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhehfl 좋은데요? ㅎㅎㅎ 👍 modern-kit에도 추가해도 되겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다! 바로 modern-kit으로 달려가겠습니다
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 네이밍이 어긋난 부분이 생각보다 많네요 @ssi02014 님 말대로 refresh이벤트에만 반응하는게 아니다보니 useBeforeUnload 가 더 적절해 보이네요
레퍼런스까지 제공해주시고 너무 감사합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/hooks/useBeforeUnload.ts
Outdated
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]); | ||
}; |
There was a problem hiding this comment.
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]);
There was a problem hiding this comment.
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
이라고 생각되네요
There was a problem hiding this comment.
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
의 사용 여부는 그냥 사용자가 필요할 때 직접!
결정할 수 있기 때문이죠 ㅎㅎ (옵셔널 옵션)
src/utils/arraysEqual.ts
Outdated
const arraysEqual = <T>(array1: T[], array2: T[]) => { | ||
return JSON.stringify(array1) === JSON.stringify(array2); | ||
}; |
There was a problem hiding this comment.
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을 참고해보세용~
There was a problem hiding this comment.
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
쪽에 있었군요 감사합니다!
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; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/utils/arraysEqual.ts
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하는 곳에서는 1차원 배열의 요소값에 대해서 순서와 값이 일치하는지만 확인하면 될 것 같아서 큰 로직 없이 간단하게 만들어 봤습니다!
함수 이름도 기존 arrayEqual
에서 내부 값만 비교한다는 뜻인 isArrayContentEqual
로 변경했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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
인 경우도 생각해서 유니온 타입으로 받는건 정말 좋은 것 같습니다!
There was a problem hiding this comment.
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)[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhehfl 타입이 완전히 호환되지 않고, 동떨어진 두 배열의 비교 케이스가 적을 것으로 예상이되고, 특별한 경우가 아니라면 타입 확장으로 잘 동작할 것으로 예상되기 때문에 any보다는 제네릭이 낫지 않을까가 제 의견입니다 ㅎㅎㅎ 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아!
어차피 제네릭으로 두어도 (string | number | boolean)[]
처럼 타입 확장이 되니 제네릭으로 두는게 더 좋을 것 같네요
확실히 any보다는 제네릭이 좋을 듯 합니다.
리뷰 감사합니다!
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]) { |
There was a problem hiding this comment.
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[]
로 수정했습니다.
🔗 관련 이슈
#46
📝작업 내용
quiz 페이지로 들어왔을때 part-id에 대한 퀴즈를 풀 수 있도록 기능을 만들었습니다.
퀴즈 진행도를 백엔드에게 전달하기 위해 api를 기다리느라 PR 올리기까지 조금 지연되었네요..
작업 내용은 크게 다음과 같습니다.
question
폴더에서 UX향상을 위한 빈칸 채우기 로직🔍 변경 사항
💬리뷰 요구사항 (선택사항)
작업을 더 세분화시켜서 이슈를 생성해야했는데 한번에 quiz에 포함되는 기능을 전부 만드려다보니 file changed가 너무 많이 생긴 것 같습니다.. 갑자기 폭탄을 떨군 것 같네요
요구사항에 관해서는 중간중간 의견을 남겨주셨으면 좋겠는 곳에 추가로 코멘트를 달아놓겠습니다
라이브러리가 여럿 추가되었는데 이에 대한 의견 부탁드립니다.