-
Notifications
You must be signed in to change notification settings - Fork 2
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: 리뷰 모아보기에서 질문별 통계 차트 구현 #803
Conversation
…ithub.com/woowacourse-teams/2024-review-me into fe/feat/773-dropdown-accordion
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.
쑤쑤~ 객관식 차트 만드느라 고생했어요! 몇 가지 제안할 사항들 있어서 코멘트 남겼으니 확인 부탁해요!!
{review.question.type === 'CHECKBOX' && review.votes !== null ? ( | ||
<DoughnutChart reviewVotes={review.votes} /> |
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.
question.type이 CHECKBOX일 때는 객관식 질문이라 반드시 review.votes가 null이 아닌 상태로 올 텐데, 꼭 필요한 부분인지 쑤쑤의 의견이 궁금해요!
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.
생각해보니! 질문 타입이 CHECKBOX면 null이 아니겠군요! 감사합니다!
const bigint = parseInt(hex.slice(1), 16); | ||
return [bigint >> 16, (bigint >> 8) & 255, bigint & 255]; |
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.
16과 255를 상수화하면 코드를 이해하는 데 더 도움이 될 것 같아요!
const colors = []; | ||
|
||
for (let i = 0; i < length; i++) { | ||
const factor = i / (length - 1); | ||
const color = interpolateColor(startColor, endColor, factor); | ||
colors.push(rgbToHex(color[0], color[1], color[2])); | ||
} | ||
|
||
return colors; |
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.
return Array.from({length}, (_, i) => {
const factor = i / (length - 1);
const interpolatedColor = interpolateColor(startColor, endColor, factor);
return rgbToHex(...interpolatedColor);
}
for문을 사용하기보다 이렇게 구현할 수도 있을 것 같아요!
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.
for문 보다는 Array.from을 사용하는 게 좋을 것 같네요!! 감사합니다!!
transition: 'opacity 0.4s ease', // 텍스트의 투명도 애니메이션 | ||
}} | ||
> | ||
{Math.floor(ratios[index] * 100)}% |
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.
지금은 floor로 항상 버림 처리되어 각 퍼센트를 다 합쳤을 때 97%로, 오차가 꽤 있어요. toFixed(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.
아.. 예리하십니다 에프이!
소수점을 바로 버리기보다는 toFixed(1)로 소수점 한 자리수까지 반올림한 값이 더 좋겠네요!!👍
import * as S from './styles'; | ||
|
||
const DOUGHNUT_COLOR = { | ||
START: '#7361DF', |
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.
START가 primary 색상이네요. theme을 불러와서 사용하면 primary에 변경이 있을때, 도넛 차트의 색상도 자동으로 바뀔 수 있어서 편할 것 같아요
}; | ||
|
||
const DoughnutChart = ({ reviewVotes }: { reviewVotes: ReviewVotes[] }) => { | ||
const radius = 90; // 차트의 반지름 |
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.
svg를 위한 상수들인 것 같아, 같이 변경되지 않는 변수들은 대문자로 변수명을 변경하는 것 어떨가요?
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 [animateIndex, setAnimateIndex] = useState(0); | ||
|
||
// 애니메이션 트리거 설정 | ||
useEffect(() => { |
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.
애니메이션 구현하느라 고생했어요.
고생한것 같아 말하지 미안하지만, 애니메이션의 필요성에 대해서는 느끼지 못하겠어요.
아코디언이 열리는 애니메이션이 있고, 만약 하나의 section에 여러 객관식이 있다면 아코디언을 열때마다 애니메이션이 실행될 경우, 눈이 피로하다?라는 느낌이 들 것 같아요(제가 지금 피곤해서 더 예민할 수도... .🥲) 성능이 느린 컴퓨터에서는 애니메이션 로딩 속도로 답답할 수 도 있을 거라는 우려도 있어요
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.
저도 애니메이션을 구현하면서 필요성에 대해 고민해 봤는데요. 생각해보니 아코디언이 펼쳐지면서 차트에도 애니메이션이 실행되니 다소 피로할 수 있을 것 같다는 느낌이 드네요. 더군다나 아코디언을 클릭할때마다 애니메이션이 실행되니까 충분히 피로할 수 있을 것 같아요.
일단, 애니메이션... Bye....
const centerX = 125; // svg의 중앙 좌표 (x) | ||
const centerY = 125; // svg의 중앙 좌표 (y) | ||
|
||
const total = reviewVotes.reduce((acc, reviewVote) => acc + reviewVote.count, 0); |
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.
total, ratio. acc 라는 변수명이 구체적이었으면 해요.
무엇에 대한 총합인지, 어떤 것에 대한 비율인지, 계산한 결과값이 무엇을 의미하는 지가 변수에 담겼으면 해요
|
||
return ( | ||
<S.DoughnutChartContainer> | ||
<svg viewBox="0 0 250 250" width="250" height="250"> |
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.
vieBox, width, height도 상수화하면 좋겠네요
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.
현란한 SVG 애니메이션 작업하느라 고생했어요 😅
사소한 제안 사항만 코멘트 남겼어요~~ 그렇게 중요한 내용은 아니지만 지금 당장 머지해야 하는 상황이 아니니 일단 RC 남기고, 나중에 알려주시면 바로 어프룹할게용
}; | ||
|
||
const DoughnutChart = ({ reviewVotes }: { reviewVotes: ReviewVotes[] }) => { | ||
const radius = 90; // 차트의 반지름 |
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.
이유는 없습니다... 대문자로 변경하는 걸 잊었을 뿐.... 😭
const ratios = reviewVotes.map((reviewVote) => reviewVote.count / total); | ||
|
||
// 누적 값 계산 | ||
const acc = reviewVotes.reduce( |
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.
단순히 내장 함수 안에서만 사용하는 값이 아니니까 acc 대신 정식 변수 이름을 줘도 될 것 같아요~!
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 acc = reviewVotes.reduce( | ||
(arr, reviewVote) => { | ||
const last = arr[arr.length - 1]; | ||
return [...arr, last + reviewVote.count]; // 현재 값과 이전 누적 값을 더해 새로운 배열 반환 | ||
}, | ||
[0], | ||
); |
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.
reduce 내부에서 누적값을 계산할 때 매번 새로운 배열을 리턴해줄 필요가 있을까요? 이렇게 스프레드 없이 간소화할 수 있을 것 같아서요~~
const acc = reviewVotes.reduce((arr, reviewVote) => {
arr.push(arr[arr.length - 1] + reviewVote.count);
return arr;
}, [0]);
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 (animateIndex < reviewVotes.length - 1) { | ||
const timer = setTimeout(() => { | ||
setAnimateIndex(animateIndex + 1); // 다음 애니메이션 트리거 | ||
}, 40); |
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.
추후 40도 상수화하면 좋을 것 같아요~~
export const Description = styled.span` | ||
${media.small} { | ||
font-size: ${({ theme }) => theme.fontSize.small}; | ||
} | ||
`; | ||
|
||
export const ReviewVoteResult = styled.span` | ||
${media.small} { | ||
font-size: ${({ theme }) => theme.fontSize.small}; | ||
} | ||
`; |
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.
이 둘 사이의 gap이 조금 더 있었으면 좋겠습니당
return ( | ||
<S.DoughnutChartContainer> | ||
<svg viewBox="0 0 250 250" width="250" height="250"> | ||
{reviewVotes.map((reviewVote, index) => { |
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.
return 반환값안에서 svg를 생성하기 위해 계산하는 코드들이 있으니 가독성이 좋지 않아요.
혹시 이를 컴포넌트로 따로 뺄 수 있을까요?
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.
구현할 때는 몰랐는데 코드를 다시 보니.... 와우.... 가독성이 정말 좋지 않네요...^^ 추후에 컴포넌트로 분리하겠습니다!
return [bigint >> 16, (bigint >> 8) & 255, bigint & 255]; | ||
}; | ||
|
||
// RGB 색상을 Hex로 변환하는 함수 |
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.
함수의 파라미터가 3개 이상이 되면, 객체로 묶는 것을 추천해요
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.
차트 덕분에 나를 파악하는 데 좀 더 도움이 될 것 같아요😊 리팩토링은 추후에...! 고생했습니다
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.
예쁜 차트 구현하느라 고생했어요
* feat: Switch 컴포넌트 제작 * chore: Switch 컴포넌트 이름 변경 * feat: 리뷰 모아보기에 대한 라우팅 추가 및 임시 페이지 구현 * feat: 리뷰 모아보기와 리뷰 목록 페이지의 공통 레이아웃 제작 * refactor: 공통 레이아웃 제작에 따른 ReviewList 리팩토링 * feat: 리뷰 목록 반응형 적용 * feat: OptionSwitch 반응형 적용 * refactor: ReviewDisplayLayout 반응형 수정 * feat: 공통 Dropdown 컴포넌트 작성 * design: 화살표 버튼 오른쪽 고정 및 옵션을 드래그하지 못하게 수정 * feat: Dropdown 외부 클릭 시 닫히도록 하는 기능 구현 * refactor: Dropdown 로직을 훅으로 분리 * chore: props 명칭 변경 및 선택된 아이템 ellipsis 처리 * feat: Accordion 공통 컴포넌트 작성 * chore: index에 Dropdown, Accordion 추가 * feat: theme에 Dropdown의 z-index 추가 * chore: 누락된 index 추가 * design: Dropdown border 색상 변경 * refactor: Accordion 로직 훅으로 분리 * fix: px을 rem으로 수정 * design: Dropdown 및 Accordion의 margin-bottom 속성 제거 * feat: 초기에 열려있는 Accordion 구현을 위해 prop 추가 * feat: 모아보기 페이지 type 정의 * feat: 모아보기 페이지 목 데이터 작성 * design: Accordion 컴포넌트에서 불필요한 props 제거 * design: Accordion 반응형 구현 * feat: 목 데이터를 사용하여 모아보기 페이지 퍼블리싱 * feat: 질문별 통계 차트 구현 * feat: 통계 차트 세부사항 표시 * feat: 질문 길이에 따라 색상을 생성하는 함수 구현 * chore: DoughnutChart 파일 위치 변경 * feat: 차트 애니메이션 적용 및 비율을 텍스트로 시각화 * chore: 비율을 'n표' 형식으로 수정 * chore: 불필요한 텍스트 제거 * feat: ReviewCollectionPage에 DoughnutChart 컴포넌트 적용 * feat: 통계 차트에 반응형 적용 * refactor: theme에 있는 primary 색상 활용 * chore: 불필요한 코드 제거 * refactor: 차트 애니메이션 제거 * chore: Accordion에 넘겨주는 props 변수명 수정 --------- Co-authored-by: ImxYJL <allensain14@gmail.com> Co-authored-by: chysis <chysiss@naver.com>
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
circle, stroke
속성을 활용하면 쉽게 구현할 수 있다는 장점이 있어 선택했습니다.애니메이션
처음엔 strokeDasharray와 strokeDashoffset에 애니메이션을 적용하면 될 거라고 생각했는데, 이렇게 하면 각 도넛 영역의 시작점에서 애니메이션이 시작되어 차례대로 나타나는 형태가 아니었습니다. 제가 원하는 건 각 영역이 부드럽게 연결되어 나타나면서, 끝나면 다음 영역이 이어지는 형태였습니다. 그래서 현재는 setTimeout을 이용해 각 영역의 애니메이션이 끝나면 다음 영역의 애니메이션이 시작되는 방식으로 구현했습니다.
2024-10-09.5.05.28.mov
색상 계산
질문 개수에 따라 시작 색상과 끝 색상 사이의 중간 색상을 계산합니다.
startHex
에는 primary 색상을,endHex
에는 옅은 보라색을 넣어 색상이 점점 더 옅어지도록 했습니다.📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말
참고로 에프이가 올리의 레이아웃 브랜치를 pull 받고, 제가 에프이의 아코디언 브랜치를 머지하면서 커밋 내역에 같이 쌓인 것 같습니다..🤔
차트 애니메이션을 적용하는 과정에서 삼각함수 내용이 등장하는데요. 전 이과지만.... 이과가 아닌 것 같습니다..^^
여러 블로그와 gpt의 도움을 많이 받았지만..... 아직 제대로 이해하지 못한 상태에서 적용하려고 하니 원하는 결과물이 나오지 않아서, 일단은 setTiemout을 이용해서 애니메이션을 적용했습니다. 생각보다 애니메이션이 끊기는 느낌이 들어서 이 부분은 추가적으로 수정이 필요할 것 같습니다.
완.벽.하.게. 차트 애니메이션에 대해 공부하고 오겠습니다!!!!! 💪
https://tecoble.techcourse.co.kr/post/2021-11-10-making-donut-chart-react/
https://evan-moon.github.io/2020/12/12/draw-arc-with-svg-clippath/