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

[FE] feat#2 채점하기 #234

Merged
merged 16 commits into from
Dec 4, 2023
Merged

[FE] feat#2 채점하기 #234

merged 16 commits into from
Dec 4, 2023

Conversation

YuHyun-P
Copy link
Member

@YuHyun-P YuHyun-P commented Dec 4, 2023

close #2

✅ 작업 내용

  • 채점 API 연결
  • 정답 시 정답 모달 띄우기
    • 링크 복사
    • 내 답안 보러가기
    • 다음 문제 풀기
  • 실패 시 토스트 에러 메시지

📸 스크린샷

정답 모달 스타일링

라이트 다크
solvedmodal light solvedmodal dark

정답 모달 링크 복사 / 다음 문제 풀기 / 내 답안 보러가기

  • 테스트 용으로 share/[slug].tsx 페이지 생성한 후 촬영했습니다!
    solvedmodal feature

마지막 문제일 때 정답 모달
image

풀이가 틀렸을 경우

라이트 다크
solve fail toast solve fail toast dark

📌 이슈 사항

  • 마지막 퀴즈 id가 하드 코딩돼서 찜찜하네요..사이드바에서 사용하는 데이터를 가져와서 마지막 id를 가져올까 고민했는데 폴더 경로가 "design-system/components/common/Sidebar/nav.ts"으로 디자인 시스템에서 가져오는 것도 이상해서 일단 useSolvedModal 훅에 하드 코딩했습니다.

🟢 완료 조건

  • 사용자는 자신의 풀이를 채점할 수 있다.
  • 답안이 맞았을 경우
    • 자신의 답안을 공유하거나 보러갈 수 있다.
    • 다음 문제를 풀 수 있다.
  • 답안이 틀렸을 경우 토스트 메시지로 확인할 수 있다.

✍ 궁금한 점

  • 현재 SolvedModal 컴포넌트는 "components/quiz"에, useSolvedModal은 "hooks"에 있습니다. 훅이 컴포넌트에 결합된 로직같은데, "components/quiz"에 넣는 건 이상할까요?
  • 공유 링크 복사 성공한 후 ✅ 이모지가 사라지는 시간이 긴 것 같은데 어떻게 생각하시나요? 요 값도 상수화하는 게 좋을까요?
  • 디자인 변경하고 싶으면 알려주세요!

@YuHyun-P YuHyun-P self-assigned this Dec 4, 2023
Copy link
Member

@yunchaehyun yunchaehyun left a comment

Choose a reason for hiding this comment

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

유현님!! 너무 고생 많으셨습니다! 최고최고🥺
Last Quiz ID는 우선 이게 최선일 것 같다는 생각이 듭니다.. ㅠㅠ
그래도 최고!! 선 Approve 할게요 :)

flexCenter,
} from "../../../design-system/tokens/utils.css";

const BORDER_RADIUS = 8;
Copy link
Member

Choose a reason for hiding this comment

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

이거 8이 정말 자주쓰여서 borderRadius가 8인 스타일 따로 빼고 싶은데, 어떠세요?

Copy link
Member Author

Choose a reason for hiding this comment

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

너무 좋습니다! 8 쓸 때마다 토큰으로 만들어야 하나 고민했어요ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

제가 지금 뺄까요?

Copy link
Member

Choose a reason for hiding this comment

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

네 부탁드릴게요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

12e0d3e 유틸 스타일로 만들었는데 적용 못하는 곳들이 있네요..!

image image

Copy link
Member

Choose a reason for hiding this comment

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

괜찮습니다! 충분히 좋아요 ㅎㅎ

]);

export const strong = style([
typography.$semantic.title3Bold,
Copy link
Member

@yunchaehyun yunchaehyun Dec 4, 2023

Choose a reason for hiding this comment

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

이거 수정해주세용🥺 title4Regular 입니다!
근데 그러면 strong 태그가 안어울릴 것 같네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

앗! 반영했습니다 76032cf


setTimeout(() => {
copyButtonRef.current?.classList.remove("visible");
}, 2000);
Copy link
Member

@yunchaehyun yunchaehyun Dec 4, 2023

Choose a reason for hiding this comment

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

해봤는데 1초가 좋을 것 같습니다! 매직넘버긴 하니까 해당 모듈 상단에 상수로만 분리해둘까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

좋아요~! c20df65에서 작업했습니다!

import * as styles from "./SolvedModal.css";

interface SolvedModalProps {
link: string;
Copy link
Member

Choose a reason for hiding this comment

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

이거 지금 slug로 수정해두는게 좋을까요?👀

Copy link
Member Author

Choose a reason for hiding this comment

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

페이지 단에서 link를 만들어서 전달하도록 변경했습니다! 767baed

링크에 base url도 필요해서 .env.local, .env.production 으로 주입하도록 했습니다. 배포 스크립트를 변경해야 하는데 제가 포크한 레포에서 테스트해보겠습니다!

// .env.local
NEXT_PUBLIC_BASE_URL=http://localhost:3000
// .env.production
NEXT_PUBLIC_BASE_URL=https://git-challenge.com

Copy link
Member

Choose a reason for hiding this comment

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

네 좋아요!! 이거 까다로울 것 같으면 내일 같이해도 좋을 것 같습니당!

Copy link
Member Author

Choose a reason for hiding this comment

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

cb6fb3f 성공했습니다! 설정하는 방법은 내일까지 개발 위키에 작성해 놓을게요..!
image

Copy link
Member

Choose a reason for hiding this comment

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

😭 고생 많으셨습니다!! 머지하셔두 돼요!

setTimeout(() => {
copyButtonRef.current?.classList.remove("visible");
}, 2000);
onCopy?.(link);
Copy link
Member

Choose a reason for hiding this comment

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

onCopy가 무슨 역할을 하는 메서드인가요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

페이지 단에서 링크 복사 성공했을 때 처리할 일이 있을까봐 만들었었는데 필요가 없네요😅 삭제했습니다!

Comment on lines 70 to 74
{!lastQuiz ? (
<Button full variant="primaryFill" onClick={onNextQuiz}>
다음 문제 풀래요
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{!lastQuiz ? (
<Button full variant="primaryFill" onClick={onNextQuiz}>
다음 문제 풀래요
</Button>
{!lastQuiz && (
<Button full variant="primaryFill" onClick={onNextQuiz}>
다음 문제 풀래요
</Button>
)}

삼항 연산자로 null을 주신 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

엇 예전에 &&로 했다가 값 자체가 렌더링되길래 이후에 삼항 연산자만 썼었는데, 테스트 해보니까 boolean이면 잘 동작하네요!! 수정하겠습니다. 감사합니다🙇‍♀️

Copy link
Member

Choose a reason for hiding this comment

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

저도 그런 경험 있어서 긴가민가해서 해봤어요 ㅎㅎ 감사합니다😊


const LAST_QUIZ_ID = 19;

export function useSolvedModal(id: number) {
Copy link
Member

Choose a reason for hiding this comment

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

useSolvedModal은 개인적으로 SolvedModal 컴포넌트 안에 있는게 더 자연스러울 것 같다고 생각해요! 컴포넌트 내부로 빼는건 찬성입니다 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

SolvedModal 폴더로 이동했습니다! 9295023

);

return {
lastQuiz: id === LAST_QUIZ_ID,
Copy link
Member

Choose a reason for hiding this comment

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

오!!! 👍🏻👍🏻👍🏻

Comment on lines 66 to 76
try {
const response = await quizAPI.submit(+id);
if (response.solved) {
solvedModal.onSolved(response.link);
return;
}
toast.error("다시 풀어보세요!");
} catch (error) {
handleResponseError(error);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

에러 처리... 저희 Error Boundary에 대해서 공부해보고 도입해보는건 어떤가요? 당장 해야할 건 아니긴한데 에러 바운더리가 저희에게 필요한게 맞다면 리팩터링에 필수적일 것 같다는 생각이 듭니다.. ㅠㅠ

Copy link
Member Author

Choose a reason for hiding this comment

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

네 좋아요👍🏻👍🏻👍🏻

@YuHyun-P YuHyun-P merged commit 96d03bc into dev-fe Dec 4, 2023
@YuHyun-P YuHyun-P deleted the feature-fe-#2 branch December 4, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants