-
Notifications
You must be signed in to change notification settings - Fork 2
[Feedback]: 로직 피드백 및 주석 추가 #62
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
base: feature/QFEED-108
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { GlobalStyles } from '@/styles/GlobalStyles'; | |
| import theme from '@/styles/theme'; | ||
| import { BottomNavigationStyleConfig as BottomNavigation } from 'chakra-ui-bottom-navigation'; | ||
|
|
||
| // [May]: 여기 정의해두셔도 괜찮지만, 따로 파일 분리해도 좋을거같습니다..! | ||
| const queryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
|
|
@@ -19,6 +20,7 @@ const queryClient = new QueryClient({ | |
| }, | ||
| }); | ||
|
|
||
| // [May]: 여기 정의해두셔도 괜찮지만, 따로 파일 분리해도 좋을거같습니다..! | ||
| const chakraTheme = extendTheme({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be better to separate the |
||
| components: { | ||
| BottomNavigation, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,20 @@ | ||
| import { useMutation } from '@tanstack/react-query'; | ||
| import { createAnswer } from '@/pages/Question/api/fetchAnswer'; | ||
| import { CreateAnswerRequest, CreateAnswerResponse } from '../types/answer'; | ||
| import { useNavigate } from 'react-router'; | ||
|
|
||
| export const useCreateAnswer = () => { | ||
| const navigate = useNavigate(); | ||
|
|
||
| return useMutation<CreateAnswerResponse, unknown, CreateAnswerRequest>({ | ||
| mutationFn: createAnswer, | ||
| onSuccess: (data) => { | ||
| console.log('Answer created successfully:', data); | ||
| }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using |
||
| // [Should] 단순히 로그를 찍는것보다 회원가입 / 로그인 페이지로 리다이렉트 시켜주는게 사용자 경험에 더 좋을거같습니다 | ||
| onError: (error) => { | ||
| console.error('Failed to create answer:', error); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using |
||
| navigate('/login'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might not be appropriate to always redirect to the login page when an error occurs. The error could be due to other reasons, not necessarily because the user is not logged in. Consider handling different types of errors differently. |
||
| }, | ||
| }); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,10 @@ export const RegisterPage = () => { | |
| return; | ||
| } | ||
|
|
||
| // [Question] 백엔드랑 합의된 로그인 / 회원가입 유효성 검사가 존재하나요? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid adding questions or suggestions as comments in the code. Instead, consider discussing these points with your team or through the PR comments section. |
||
| // [Should] 존재한다면 zod등을 이용해서 유효성 검사 및 사용자에게 피드백(비밀번호가 형식에 안맞음 등,,)을 알려주는게 좋을거같습니다. | ||
| // 현재 비밀번호 유효성 검사에 실패하여 회원가입이 실패되는데, 유저 입장에서는 무었때문에 거절 당한지 알수가없습니다 | ||
|
|
||
| // 모든 조건이 충족되면 로컬스토리지에 저장 | ||
| localStorage.setItem('registerEmail', data.email); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Storing sensitive information like passwords in local storage is not secure. Consider using more secure methods to handle passwords. |
||
| localStorage.setItem('registerPassword', data.password); | ||
|
|
||
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.
Consider moving the definition of
queryClientto a separate file. This will help in maintaining the code and improving readability.