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

[FIX] 질문에 대한 도메인 용어를 question에서 content로 수정 #44

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

rbgksqkr
Copy link
Contributor

@rbgksqkr rbgksqkr commented Jul 22, 2024

Issue Number

#43

As-Is

  • 질문에 해당하는question이 option과 비슷하게 생겨 가독성에 악영향

To-Be

  • 질문 도메인 용어를 question에서 content로 수정

Check List

  • 테스트가 전부 통과되었나요?
  • 모든 commit이 push 되었나요?
  • merge할 branch를 확인했나요?
  • Assignee를 지정했나요?
  • Label을 지정했나요?

Test Screenshot

image

(Optional) Additional Description

@rbgksqkr rbgksqkr added 🛠 fix 버그 🫧 FE front end labels Jul 22, 2024
@rbgksqkr rbgksqkr added this to the FE Sprint2 milestone Jul 22, 2024
@rbgksqkr rbgksqkr requested review from useon and novice0840 July 22, 2024 08:49
@rbgksqkr rbgksqkr self-assigned this Jul 22, 2024
@rbgksqkr rbgksqkr linked an issue Jul 22, 2024 that may be closed by this pull request
2 tasks
Copy link
Contributor

@novice0840 novice0840 left a comment

Choose a reason for hiding this comment

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

빠른 요구 사항 수정 좋아요 !!

대부분은 변수명 변경이라 이부분은 넘어가도 될거 같아서 피드백 생략할게요

Comment on lines +9 to +21
# 브랜치 패턴을 변수로 선언
branch_patterns=("feat/" "hotfix/" "fix/" "refactor/" "test/")

# 브랜치가 패턴에 맞는지 확인하는 함수
matches_pattern() {
local branch=$1
for pattern in "${branch_patterns[@]}"; do
if [[ $branch == $pattern* ]]; then
return 0
fi
done
return 1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

쉘 스크립트의 함수랑 반복문은 사실 처음 봐서 대략 감으로 유추하자면
return 0 정상 동작이고 return 1이 나오면 비정상 동작으로 에러가 발생하는 함수인 것 같군요.

모든 경우의 브랜치 이름 확인하는 거 좋네요 ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

지피티 참고하긴했는데, 기존 로직대로 추가하면 if문이 많아져서 나중에 브랜치 전략이 변경되어도 수정하기 편하도록 정규표현식으로 작성해보았습니다!!

Comment on lines +6 to +12
export const fetchBalanceContent = async (roomId = 1): Promise<BalanceContent> => {
const res = await fetcher.get({ url: API_URL.balanceContent(roomId) });

const data = await res.json();

return data;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

일단 API가 나오지 않아서 roomId의 기본값을 1로 넣은 건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

백엔드와 협의할 때 일단 1을 넣어 API 보내라고 해서 설정한 값입니다!!!


const RoundVoteResult = () => {
const { data: question } = useQuestionQuery();
const { balanceContent } = useBalanceContentQuery();
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

data를 question으로 받는 형식이 아닌 balanceContent로 변경한 이유가 무엇인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useQuery를 그대로 반환하고, data는 사용처에서 적절하게 변수명 바꿔주는 게 확장성 측면에서 좋다고 생각하였습니다!
하지만 훅 특성상 여러 상황에서 쓰인다기보단 고정적으로 balanceContent로만 사용되기 때문에 변수값을 고정적으로 설정하였습니다!

@rbgksqkr rbgksqkr merged commit 87ae571 into develop Jul 22, 2024
1 check passed
@GIVEN53 GIVEN53 deleted the fix/#43 branch December 10, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🫧 FE front end 🛠 fix 버그
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FIX] 질문에 대한 도메인 용어를 question에서 content로 수정
3 participants