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(view): Vertical Cluster List 에서 사용자들 추가 제한하는 방식으로 수정 (#524) #570

Closed
wants to merge 0 commits into from

Conversation

HIITMEMARIO
Copy link
Contributor

Related issue

#524

Result

선택된 사용자가 10명 이상이라면 추가 인원을 + more 로 표시 시키고 가장 최근에 선택된 10명만 리스트에 보이게 하도록 수정하였습니다.

Work list

image

Discussion

스크롤 방식도 있겠지만 UI가 지저분 해지지 않을까 싶기도 했고 단순히 보여주는 것 외에 큰 기능이 없었기 때문에 선택된 사용자는 chart에서 확인 해도 된다고 생각하여 사용자를 전부 보여주는것이 아니라 일부만 표시하게 구현하였습니다...! 혹시 더 좋은 생각이 있으시다면 언제든 환영입니다...!!😄

@HIITMEMARIO HIITMEMARIO self-assigned this Jul 30, 2024
@HIITMEMARIO HIITMEMARIO requested review from a team as code owners July 30, 2024 21:14
@seungineer
Copy link
Member

10명 이상인 경우 + n more로 표시되는 방식이 훨씬 더 깔끔해 보이네요 😮❗

Vertical Cluster List를 클릭하면 프로필 이미지가 추가되는 이유가 현재 선택한 Cluster에 기여한 인원을 한 눈에 파악하기 위함인 것 같아서 '프로필 이미지 추가 제한 방식'에 아래 로직도 추가하면 어떤가 하여 의견 말씀드려요~

Vertical Cluster List 클릭 시 프로필 이미지가 추가될 때 이미 프로필이 떠 있는 사용자라면 추가되지 않도록 제한하면 더 좋을 것 같아요.


다른 분들은 어떻게 생각하시나요? 🤔🤔🤔🤔
프로필 이미지가 나오는 의도가 Cluster를 클릭한 순서대로 참여한 인원을 보여주는 것이라면 현재 방식이 맞는 것 같은데,, 판단이 잘 안 되네요.. 🫠

@HIITMEMARIO
Copy link
Contributor Author

10명 이상인 경우 + n more로 표시되는 방식이 훨씬 더 깔끔해 보이네요 😮❗

Vertical Cluster List를 클릭하면 프로필 이미지가 추가되는 이유가 현재 선택한 Cluster에 기여한 인원을 한 눈에 파악하기 위함인 것 같아서 '프로필 이미지 추가 제한 방식'에 아래 로직도 추가하면 어떤가 하여 의견 말씀드려요~

Vertical Cluster List 클릭 시 프로필 이미지가 추가될 때 이미 프로필이 떠 있는 사용자라면 추가되지 않도록 제한하면 더 좋을 것 같아요.

다른 분들은 어떻게 생각하시나요? 🤔🤔🤔🤔 프로필 이미지가 나오는 의도가 Cluster를 클릭한 순서대로 참여한 인원을 보여주는 것이라면 현재 방식이 맞는 것 같은데,, 판단이 잘 안 되네요.. 🫠

사실 저도 해당 기능의 목적이 정확히 어떤 것인지 약간 애매한 부분이 있어 임의대로 판단하여 구현하게 되었습니다...! 감사합니다ㅎㅎ 다른 의견들도 궁금하네요..!!😁
"Vertical Cluster List 클릭 시 프로필 이미지가 추가될 때 이미 프로필이 떠 있는 사용자라면 추가되지 않도록 제한하면 더 좋을 것 같아요." 이 부분은 좋은 생각인거 같습니다...!! 한번 수정해볼게요!!!

const selectedClustersLength = selectedClusters.length - filteredSelectedData.length;

// 이미 선택된 사용자를 관리
const addedAuthors = new Set();
Copy link
Contributor

Choose a reason for hiding this comment

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

이미 추가된 사용자라면 나오지 않게 하는 방식이 저도 좋은 것 같다고 생각합니다! Set을 활용하는 방법 배워갑니다,,🙂👍👍

Copy link
Contributor

Choose a reason for hiding this comment

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

xxxjinn
xxxjinn previously approved these changes Aug 1, 2024

return (
<div className="selected-container">
{authSrcMap &&
selectedClusters.map((selectedCluster) => {
filteredSelectedData.reverse().map((selectedCluster) => {
Copy link
Contributor

@xxxjinn xxxjinn Aug 1, 2024

Choose a reason for hiding this comment

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

(궁금합니다..!) 12번째 줄에서 reverse를 했는데 여기서 또 reverse를 하는 이유가 있을까요?? 그리고 map함수라면 어차피 전체 배열을 순회하는데 reverse를 하신 이유도 궁금합니다!

@ytaek
Copy link
Contributor

ytaek commented Aug 1, 2024

10명 이상인 경우 + n more로 표시되는 방식이 훨씬 더 깔끔해 보이네요 😮❗
Vertical Cluster List를 클릭하면 프로필 이미지가 추가되는 이유가 현재 선택한 Cluster에 기여한 인원을 한 눈에 파악하기 위함인 것 같아서 '프로필 이미지 추가 제한 방식'에 아래 로직도 추가하면 어떤가 하여 의견 말씀드려요~
Vertical Cluster List 클릭 시 프로필 이미지가 추가될 때 이미 프로필이 떠 있는 사용자라면 추가되지 않도록 제한하면 더 좋을 것 같아요.
다른 분들은 어떻게 생각하시나요? 🤔🤔🤔🤔 프로필 이미지가 나오는 의도가 Cluster를 클릭한 순서대로 참여한 인원을 보여주는 것이라면 현재 방식이 맞는 것 같은데,, 판단이 잘 안 되네요.. 🫠

사실 저도 해당 기능의 목적이 정확히 어떤 것인지 약간 애매한 부분이 있어 임의대로 판단하여 구현하게 되었습니다...! 감사합니다ㅎㅎ 다른 의견들도 궁금하네요..!!😁 "Vertical Cluster List 클릭 시 프로필 이미지가 추가될 때 이미 프로필이 떠 있는 사용자라면 추가되지 않도록 제한하면 더 좋을 것 같아요." 이 부분은 좋은 생각인거 같습니다...!! 한번 수정해볼게요!!!

저도 이거 좋은 생각 같습니다!!
10명이라는 수가 임의의 수라서 조금 애매한데,
의견주신대로 중복제거해서 나오는 author 수라면 별로 많지 않을 수도 있을 것 같네요!!

(사실 지금 저렇게 뿌려주는 UX 도 애매하긴 합니다만... ㅎㅎ)

Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

중복제거를 한 결과로 나온다면, 10으로 줄여야할 이유도 좀 없어질 것 같은데요.
10으로 해야 하는 명확한 이유가 있는게 아니라면
중복제거한 채로 기존처럼 다 뿌려줘도 괜찮을 것 같습니다.

@@ -9,21 +9,36 @@ const FilteredAuthors = () => {
const { selectedData } = useGlobalData();
const authSrcMap = usePreLoadAuthorImg();
const selectedClusters = getInitData(selectedData);
const filteredSelectedData = selectedClusters.reverse().slice(0, 9);
Copy link
Contributor

Choose a reason for hiding this comment

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

10이라는 숫자는 변수화 시켜주는게 좋을 것 같습니다!
그리고, 숫자 10에 대한 명확한 근거가 있는게 아니라면, 해당 수치의 정당성(?)에 대해 고민은 좀 해봐야할 것 같습니다.

@lxxmnmn
Copy link
Contributor

lxxmnmn commented Aug 2, 2024

중복제거를 한 결과로 나온다면, 10으로 줄여야할 이유도 좀 없어질 것 같은데요. 10으로 해야 하는 명확한 이유가 있는게 아니라면 중복제거한 채로 기존처럼 다 뿌려줘도 괜찮을 것 같습니다.

그렇네요! 중복을 제거한다면 대개는 author 수가 그렇게까지 많지 않을 것 같아요
UX 리뉴얼에서 이 부분을 좀 더 깔끔하고 효과적으로 표현할 수 있는 방법을 고민해보면 좋을 것 같습니다 🤔🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants