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.

Sign up for GitHub

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

[view] 선택된 클러스터의 정보 표시 #664

Merged
merged 6 commits into from
Aug 25, 2024
Merged

[view] 선택된 클러스터의 정보 표시 #664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
318ee6a
style(view): 선택한 클러스터 하이라이팅
lxxmnmn Aug 22, 2024
4b2ce4d
fix(view): 불필요한 div 제거
lxxmnmn Aug 22, 2024
33a9ae8
style(view): 하이라이팅 스타일 수정
lxxmnmn Aug 22, 2024
0cd5ae6
feat(view): 선택된 클러스터 드롭다운 리스트 추가
lxxmnmn Aug 23, 2024
e3406fa
Merge branch 'githru:main' into main
lxxmnmn Aug 23, 2024
2832c04
Merge branch 'githru:main' into main
lxxmnmn Aug 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions packages/view/src/components/Detail/Detail.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
.detail__summary__container {
display: flex;
justify-content: space-between;
margin: 15px 0 20px;
align-items: center;
font-size: $font-size-caption;

Expand Down Expand Up @@ -49,7 +48,7 @@
display: flex;
flex-direction: column;
row-gap: 7px;
padding: 0 20px;
padding: 20px;
font-size: $font-size-body;

.commit-item {
Expand Down Expand Up @@ -92,7 +91,7 @@
overflow: visible;
position: absolute;
z-index: 10;
background-color: #212121;
background-color: lighten($gray-900, 5);
padding-bottom: 5px;
top: -7px;
}
Expand Down Expand Up @@ -125,7 +124,7 @@

.toggle-button {
display: flex;
margin: 20px auto 0;
margin: 0 auto;
border: none;
background: none;
color: var(--primary-color);
Expand Down
16 changes: 2 additions & 14 deletions packages/view/src/components/FilteredAuthors/FilteredAuthors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,8 @@
@import "styles/app";

.selected-container {
.selected__author {
display: flex;
align-items: center;
gap: 15px;
width: 100%;

p {
font-size: $font-size-title;
font-weight: $font-weight-semibold;
}
}

.selected-content {
display: flex;
flex-wrap: wrap;
box-sizing: border-box;
align-items: center;
gap: 5px;
}
6 changes: 3 additions & 3 deletions packages/view/src/components/FilteredAuthors/FilteredAuthors.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ const FilteredAuthors = () => {
const addedAuthors = new Set();

return (
<div className="selected-container">
{selectedClusters.length > 0 && <p>Authors:</p>}
<div className="selected-content">
<div className="selected__content">
Copy link
Contributor

Choose a reason for hiding this comment

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

@pcwadarong 님하고도 잠깐 얘기한 것 같은데,
이번 기회에 className등의 scss 관련 naming convention 도 정하면 좋을 것 같습니다.
해당 부분 이슈로 생성부탁드리겠습니다!
(아 이미 만들어져있나요? ;; )

<p>Authors:</p>
<div className="selected__author">
{authSrcMap &&
selectedClusters.map((selectedCluster) => {
return selectedCluster.summary.authorNames.map((authorArray: string[]) => {
Expand Down
22 changes: 22 additions & 0 deletions packages/view/src/components/FilteredClusters/FilteredClusters.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
@import "styles/app";

.selected__cluster {
& .MuiMenu-paper {
max-height: 250px;
overflow-y: auto;
}

& .MuiMenu-list {
display: flex;
flex-direction: column;
padding: 5px;
}

& .MuiChip-root {
width: 200px;
text-overflow: ellipsis;
justify-content: space-between;
margin: 5px;
background-color: $gray-300;
}
}
72 changes: 72 additions & 0 deletions packages/view/src/components/FilteredClusters/FilteredClusters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { useState } from "react";
import type { MouseEvent } from "react";
import Button from "@mui/material/Button";
import Menu from "@mui/material/Menu";
import Chip from "@mui/material/Chip";
import ArrowDropDownRoundedIcon from "@mui/icons-material/ArrowDropDownRounded";

import { selectedDataUpdater } from "components/VerticalClusterList/VerticalClusterList.util";
import { getInitData, getClusterById } from "components/VerticalClusterList/Summary/Summary.util";
import { useGlobalData } from "hooks";

import "./FilteredClusters.scss";

const FilteredClusters = () => {
const { selectedData, setSelectedData } = useGlobalData();
const selectedClusters = getInitData(selectedData);

const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

El 이 Element를 얘기하는거라면, full name으로 적어주시면 감사하겠습니다.

Copy link
Member

Choose a reason for hiding this comment

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

@ytaek
mui Menu의 props 명이 anchorEl이라 mui에 사용되는 state인지 쉽게 알 수 있어서 저는 full name이 아니더라도 좋은 것 같더라구요 🧐

props로만 사용되는 경우(지금 같은 상황!)에도 가급적 full name으로 적는게 코드 유지보수 관점에서 더 나은 건가요? 궁금합니다! 💭💭

Copy link
Contributor

Choose a reason for hiding this comment

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

@ytaek mui Menu의 props 명이 anchorEl이라 mui에 사용되는 state인지 쉽게 알 수 있어서 저는 full name이 아니더라도 좋은 것 같더라구요 🧐

그러네요. 지금보니 MUI Element의 Menu component에 딸려있는 mui 고유값이군요.
이럴 경우 조금 애매하긴 하네요 😺

방금까지 적은 답변은 이랬는데,

저희가 만약 MUI 말고 다른 UI framework 을 사용하게 된다면, 
UI와 관계없는 state값의 변수이름도 변경을 해줘야 하지 않을까요?
재사용성이란건 결국 의존성을 잘 분리한다는 얘기이기 때문에,
결국 현재 기준에서 관계가 없는(의존하지 않는) 부분에 부가적인 의존성을 달지 않도록 하는게 좋구요.

라고 쓰고 보니 MUI 의존성이 높은 코드라서, 어차피 MUI를 안 쓰면 컴포넌트가 통쨰로 변경될 것 같은 느낌적인 느낌이군요 :)
원론적으로는 full name으로 element라고 하면 쉽게 알아들을 수 있어서 좋다 까지만 얘기해야겠군요!

어쨋든 저는 처음에 anchorEl 이라고 적힌 변수명이 이해가 안가서 리뷰 시간이 약간 더 느려지긴 했으니까요 (5초 정도? ㅋㅋ)

props로만 사용되는 경우(지금 같은 상황!)에도 가급적 full name으로 적는게 코드 유지보수 관점에서 더 나은 건가요? 궁금합니다! 💭💭

매우 잘 통용되는 term 들 (예를 들어 Identification => ID) 등이 아니라면 가급적 full로 풀어쓰는걸 권장하는 추세입니다.
옛날에 변수명 길이 제한이 있다거나, 변수 자동완성 같은 IDE 기능들이 없을 때라면 모르지만,
지금은 full name으로 안할 이유가 사실 없긴 하죠. 😸

너무 길거나, 아니면 용어집 or 유비쿼터스 언어(도메인 주도 개발 참조)에서 약어로 결정하지 않은,
다시 말하면 전체 개발팀의 협의가 이뤄진게 아니라면 (다른 사람이 봐서 이해를 못 할 가능성이 있다면)
원칙을 지키는게 좋을 것입니다.

변수 이름 하나가지고 좀 장황하게 말하긴 했는데 😈
이런게 하나하나 모여서 코드의 가독성을 결정하는 것이니까요 ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

확실히 MUI Menu의 prop이라는 걸 모르는 상황에서는 알아보기 쉽지 않을 것 같네요 😅
더 직관적인 변수명으로 수정해보겠습니다!

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
Contributor

Choose a reason for hiding this comment

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

일단 머지하고, 이슈 등록해놓고 정리해주셔도 좋을 것 같습니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 그러면 우선 머지하고 전반적인 naming에 대한 이슈 등록해두겠습니다!

const isOpen = Boolean(anchorEl);

const openSelectedList = (event: MouseEvent<HTMLButtonElement>) => {
setAnchorEl(event.currentTarget);
};

const closeSelectedList = () => {
setAnchorEl(null);
};

const deselectCluster = (clusterId: number) => () => {
const selected = getClusterById(selectedData, clusterId);
setSelectedData(selectedDataUpdater(selected, clusterId));
};

return (
<div className="selected__content">
<Button
id="selected-list-button"
aria-controls={isOpen ? "selected-list" : undefined}
aria-expanded={isOpen ? "true" : undefined}
aria-haspopup="true"
sx={{ color: "inherit", padding: 0, textTransform: "none" }}
onClick={openSelectedList}
>
<p>Clusters</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Cluster라는 코드 용어보다는 사용자가 이해할 수 있는 단어로 표현하면 좋을 것 같습니다.
거기에 "selected"를 붙이면 더 좋겠구요. Selected Nodes 정도?

<ArrowDropDownRoundedIcon />
</Button>
<Menu
className="selected__cluster"
id="selected-list"
anchorEl={anchorEl}
open={isOpen}
MenuListProps={{
"aria-labelledby": "selected-list-button",
}}
onClose={closeSelectedList}
>
{selectedClusters.map((selectedCluster) => {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

(사소) return 없어도 괜찮지 않을까요? 😄

<li key={selectedCluster.clusterId}>
<Chip
label={selectedCluster.summary.content.message}
Copy link
Contributor

Choose a reason for hiding this comment

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

추~~~후 고민해보면 좋을 요소들 나열해봅니다.
이슈로 등록해주시고, 추후에 한번 view팀원분들과 같이 얘기해봐도 좋을 것 같아요.

  • 지금 label 표시란이 좀 짧은데, message가 들어가면 충분할지,
  • 아니면 다른 ID 같은 걸 표시할만한게 있는지 (commit ID같은?)
  • 그리고, selection이 보여주는 부분에 sorting이 필요할지
  • 만약에 ID 등으로 구분이 안되는 거라면, 해당 chip selected box 부분이 필요하지 않을 수도 있구요.
  • 삭제(deselection) 기능이 좋긴 한데, 정말 필요한 부분일까?
  • select box의 item을 눌렀을 때 해당 cluster로 스크롤링 하는건 어떨까?

좀 디테일하긴 한데, 실제 UI 디자인에 대한 고민 practice라고 생각하셔도 좋습니다 😺

Copy link
Contributor Author

Choose a reason for hiding this comment

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

우와 전부 좋은 이슈들이네요!
특히 message 길이가 꽤 길어서 다른 요소를 사용하면 좋겠다 싶었는데 commit ID도 괜찮을 것 같습니다

제안해주신 내용은 UI Renewal 팀 회의에서 논의해보겠습니다! 🫡

onDelete={deselectCluster(selectedCluster.clusterId)}
/>
</li>
);
})}
</Menu>
</div>
);
};

export default FilteredClusters;
1 change: 1 addition & 0 deletions packages/view/src/components/FilteredClusters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as FilteredClusters } from "./FilteredClusters";
Copy link
Contributor

Choose a reason for hiding this comment

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

이게 Filtering된건지, Selection(선택)된건지 헷갈리는데,
naming을 명확히 하면 좋을 것 같습니다. select이 맞는것 같아 보이는데
(scss에는 selected_cluster라고 되어 있으니까요)

Copy link
Contributor Author

@lxxmnmn lxxmnmn Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ytaek
기존에 존재하던 FilteredAuthors 컴포넌트와 목적이 동일하기에 "Filtered"를 차용했습니다!
두 컴포넌트 모두 select 되었음을 나타내는 이름으로 변경하는 게 좋을까요?

Copy link
Contributor

@ytaek ytaek Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ytaek 기존에 존재하던 FilteredAuthors 컴포넌트와 목적이 동일하기에 "Filtered"를 차용했습니다! 두 컴포넌트 모두 select 되었음을 나타내는 이름으로 변경하는 게 좋을까요?

아. 그랬군요.
그런데, 현재 Author같은 경우는, 사용자의 "selection"으로 filtering된건 아니지 않나요?
(selection한 cluster라는 조건으로 filtering된 author list 같은 느낌인데)
지금 수정한 컴포넌트는 cluster 들이 "selection" 된 것들이구요.

정확하게 동일한 부분이면 통일하는게 맞을것 같구요.
그렇지 않다면 구분하면 좋을 것 같습니다.

"selection"이 맞고 "filter"가 틀리다는 얘기라기 보다,
같은 기능은 같은 단어/표현을 쓰자 에 포커스 된 말이라고 보시면 좋을 것 같습니다 🦈

Copy link
Contributor

Choose a reason for hiding this comment

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

(사소하지만 또 생각나서) 말씀듣고보니,

FilteredAuthors는 단순히 author list만 보여주는 것이고,
Filtered(?Selected)Clusters는 단순 리스트를 보여준다기보다 현재 list를 삭제하는 기능도 들어가있는 컴포넌트네요!

목적이나 기능이 현재이름에서 뭔가 더 들어가면 좋을 것 같긴합니다 : )
SelectedClusterListBox 라던지? ㅎㅎ

뭔가 이번 리뷰는 naming에 진심인 리뷰가 되었는데,
이번 기회에 한번 요런걸 더 고민해보면 좋겠다! 라는 취지로 이해해주시면 더 좋을 것 같습니다 : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

클러스터 노드를 선택했을 때 보여주다보니 FilteredAuthors의 의미를 잘못 이해했네요 ㅎㅎ,,
목적과 기능을 잘 나타낼 수 있는 naming에 대해 더 고민하고 수정해보겠습니다
꼼꼼한 리뷰 감사합니다!!! ✨

Copy link
Member

Choose a reason for hiding this comment

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

뭔가 이번 리뷰는 naming에 진심인 리뷰가 되었는데,
이번 기회에 한번 요런걸 더 고민해보면 좋겠다! 라는 취지로 이해해주시면 더 좋을 것 같습니다 : )

엄청 엄청 유익했습니다 🔥🔥🔥🔥🔥🔥

17 changes: 13 additions & 4 deletions packages/view/src/components/VerticalClusterList/Summary/Summary.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,19 @@
display: flex;
flex-direction: column;
flex-grow: 1;
padding-left: 20px;
padding-top: 5px;
padding-left: 10px;
width: 100%;
height: 100%;
overflow-x: hidden;
overflow-y: scroll;

.cluster-summary__cluster {
display: flex;
align-items: center;
column-gap: 10px;

.toggle-contents-button {
padding: 5px 15px;
padding: 10px 15px;
border: none;
background-color: transparent;
cursor: pointer;
Expand All @@ -28,6 +32,11 @@
display: flex;
flex-direction: column;
width: 100%;

&.selected {
border-radius: 25px;
background-color: lighten($gray-900, 5);
}
}

.toggle-contents-container {
Expand Down Expand Up @@ -100,7 +109,7 @@
overflow: auto;
height: 220px;
max-height: 280px;
padding: 0px 30px;
padding: 10px 30px;
}

.sort-button {
Expand Down
6 changes: 5 additions & 1 deletion packages/view/src/components/VerticalClusterList/Summary/Summary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ const Summary = () => {
clusterSizes={[clusterSizes[index]]}
/>
</div>
<div className="cluster-summary__info-wrapper">
<div
className={`cluster-summary__info-wrapper ${
selectedClusterId.includes(cluster.clusterId) ? "selected" : ""
}`}
>
Comment on lines +52 to +56
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 조건에 맞는 경우에만 class 명을 추가할 수도 있군요 😲

selected된 cluster summary info 임을 쉽게 알 수 있네요(대박) 👍

<button
type="button"
className="toggle-contents-button"
Expand Down
24 changes: 19 additions & 5 deletions packages/view/src/components/VerticalClusterList/VerticalClusterList.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,26 @@
@import "styles/app";

.vertical-cluster-list {
display: flex;
flex-direction: column;
}

.vertical-cluster-list__content {
.selected__container {
display: flex;
height: 100%;
overflow-x: hidden;
overflow-y: scroll;
flex-grow: 1;
justify-content: space-between;
align-items: center;
column-gap: 20px;
padding: 0 10px 20px;
width: 100%;

& .selected__content {
display: flex;
align-items: center;
column-gap: 10px;

p {
font-size: $font-size-title;
font-weight: $font-weight-semibold;
}
}
}
19 changes: 13 additions & 6 deletions packages/view/src/components/VerticalClusterList/VerticalClusterList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
import "./VerticalClusterList.scss";

import { useGlobalData } from "hooks";
import { FilteredAuthors } from "components/FilteredAuthors";
import { FilteredClusters } from "components/FilteredClusters";

import { Summary } from "./Summary";

import "./VerticalClusterList.scss";

const VerticalClusterList = () => {
const { selectedData } = useGlobalData();

return (
<div className="vertical-cluster-list">
<FilteredAuthors />
<div className="vertical-cluster-list__content">
<Summary />
</div>
{selectedData.length > 0 && (
<div className="selected__container">
<FilteredAuthors />
<FilteredClusters />
</div>
)}
<Summary />
Comment on lines +14 to +20
Copy link
Member

Choose a reason for hiding this comment

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

훨씬 일목요연해진 것 같습니다!! 🫢🤭

</div>
);
};
Expand Down
Loading