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

lxxmnmn · 2024-08-23T06:50:06Z

Related issue

#564

Result

Work list

1. 선택된 클러스터 강조

선택된 클러스터를 더 명확하게 구분할 수 있도록 배경 색상을 부여하고 여백을 조절했습니다.

2. 선택된 클러스터 정보 표시

선택된 클러스터의 정보를 모아서 보여주기 위해 Summary 목록 상단에 FilteredClusters 컴포넌트를 추가했습니다.

"Clusters" 버튼을 클릭하면 드롭다운 리스트가 보여집니다.
리스트 항목의 ❌ 버튼을 클릭하면 선택을 해제할 수 있습니다.

3. 태그 구조 변경

FilteredClusters 컴포넌트를 추가함에 따라 HTML 태그 구조를 변경했습니다.

Summary 컴포넌트를 불필요하게 감싸고 있던 div를 제거했습니다.
목적이 동일한 FilteredAuthors와 FilteredClusters를 하나의 div에 담았습니다.
- 이 컨테이너는 선택된 클러스터가 있을 경우에만 표시합니다.

Discussion

클러스터를 일괄 선택 해제하는 기능을 나중에 추가해도 좋을 것 같습니다.
message의 길이가 대체로 길기 때문에 여러 개를 선택할 경우를 고려하여 드롭다운 리스트 형태로 구현했습니다.
더 적합한 UI가 있다면 편하게 제안해주세요! 👐

ytaek

사용자의 선택을 명시적으로 보여주는 아주 필요했던 기능인 것 같습니다!!! 👍👍👍

몇가지 comment남겼는데, 주로 naming에 관한 comment 이고,
요즘은 별도 주석없이 코드에 context를 다 녹여서 얘기하자.. 코드로 말하자.. 이런 추세이기도 하고
타인이 코드 해석할 때에도 이름으로 기능들을 유추하기 때문에,
naming은 조금 신경써서 작업하면 좋을 것 같습니다 😸

ytaek · 2024-08-23T06:55:38Z

packages/view/src/components/FilteredAuthors/FilteredAuthors.tsx

-    <div className="selected-container">
-      {selectedClusters.length > 0 && <p>Authors:</p>}
-      <div className="selected-content">
+    <div className="selected__content">


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

ytaek · 2024-08-23T06:56:18Z

packages/view/src/components/FilteredClusters/FilteredClusters.tsx

+  const { selectedData, setSelectedData } = useGlobalData();
+  const selectedClusters = getInitData(selectedData);
+
+  const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null);


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

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

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

@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 유비쿼터스 언어(도메인 주도 개발 참조)에서 약어로 결정하지 않은,
다시 말하면 전체 개발팀의 협의가 이뤄진게 아니라면 (다른 사람이 봐서 이해를 못 할 가능성이 있다면)
원칙을 지키는게 좋을 것입니다.

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

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

오오 그렇군요! 흥미롭네요 👍

변수명 짓는데 시간이 많이 드는 이유가 조금이나마 이해됐습니다🫡
자세한 답변 감사합니다!

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

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

ytaek · 2024-08-23T06:57:58Z

packages/view/src/components/FilteredClusters/FilteredClusters.tsx

+        onClose={closeSelectedList}
+      >
+        {selectedClusters.map((selectedCluster) => {
+          return (


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

ytaek · 2024-08-23T07:01:03Z

packages/view/src/components/FilteredClusters/index.ts

@@ -0,0 +1 @@
+export { default as FilteredClusters } from "./FilteredClusters";


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

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

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

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

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

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

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

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

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

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

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

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

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

seungineer

수정된 부분이 많아 보여 살짝 쫄았는데, 하나 하나 읽어보니 엄청 잘 읽혔어요 👍👍
LGGGGTM 🚀🚀🚀🚀

seungineer · 2024-08-23T11:40:02Z

packages/view/src/components/VerticalClusterList/VerticalClusterList.tsx

+      {selectedData.length > 0 && (
+        <div className="selected__container">
+          <FilteredAuthors />
+          <FilteredClusters />
+        </div>
+      )}
+      <Summary />


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

seungineer · 2024-08-23T11:43:02Z

packages/view/src/components/VerticalClusterList/Summary/Summary.tsx

+            <div
+              className={`cluster-summary__info-wrapper ${
+                selectedClusterId.includes(cluster.clusterId) ? "selected" : ""
+              }`}
+            >


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

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

seungineer · 2024-08-23T12:07:17Z

packages/view/src/components/FilteredClusters/FilteredClusters.tsx

+  const { selectedData, setSelectedData } = useGlobalData();
+  const selectedClusters = getInitData(selectedData);
+
+  const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null);


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

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

ytaek

몇가지 comment들 더 추가했습니다.

ytaek · 2024-08-23T12:54:38Z

packages/view/src/components/FilteredClusters/FilteredClusters.tsx

+        sx={{ color: "inherit", padding: 0, textTransform: "none" }}
+        onClick={openSelectedList}
+      >
+        <p>Clusters</p>


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

ytaek · 2024-08-23T13:03:19Z

packages/view/src/components/FilteredClusters/FilteredClusters.tsx

+          return (
+            <li key={selectedCluster.clusterId}>
+              <Chip
+                label={selectedCluster.summary.content.message}


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

지금 label 표시란이 좀 짧은데, message가 들어가면 충분할지,

아니면 다른 ID 같은 걸 표시할만한게 있는지 (commit ID같은?)

그리고, selection이 보여주는 부분에 sorting이 필요할지

만약에 ID 등으로 구분이 안되는 거라면, 해당 chip selected box 부분이 필요하지 않을 수도 있구요.

삭제(deselection) 기능이 좋긴 한데, 정말 필요한 부분일까?

select box의 item을 눌렀을 때 해당 cluster로 스크롤링 하는건 어떨까?

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

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

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

#664 refactoring * fix(view): 선택된 클러스터 목록 컴포넌트 naming 수정 * fix(view): 불필요한 return 제거

githru#664 refactoring * fix(view): 선택된 클러스터 목록 컴포넌트 naming 수정 * fix(view): 불필요한 return 제거

lxxmnmn added 5 commits August 22, 2024 15:30

style(view): 선택한 클러스터 하이라이팅

318ee6a

fix(view): 불필요한 div 제거

4b2ce4d

style(view): 하이라이팅 스타일 수정

33a9ae8

feat(view): 선택된 클러스터 드롭다운 리스트 추가

0cd5ae6

Merge branch 'githru:main' into main

e3406fa

lxxmnmn added ✨ view new feature labels Aug 23, 2024

lxxmnmn added this to the v0.7.1 milestone Aug 23, 2024

lxxmnmn self-assigned this Aug 23, 2024

lxxmnmn requested review from a team as code owners August 23, 2024 06:50

ytaek approved these changes Aug 23, 2024

View reviewed changes

seungineer approved these changes Aug 23, 2024

View reviewed changes

ytaek reviewed Aug 23, 2024

View reviewed changes

Merge branch 'githru:main' into main

2832c04

lxxmnmn merged commit f3546aa into githru:main Aug 25, 2024
2 checks passed

lxxmnmn mentioned this pull request Aug 27, 2024

[view] 선택된 클러스터 컴포넌트 수정 #676

Merged

lxxmnmn added a commit that referenced this pull request Aug 29, 2024

[view] 선택된 클러스터 컴포넌트 수정 (#676)

eab73b2

#664 refactoring * fix(view): 선택된 클러스터 목록 컴포넌트 naming 수정 * fix(view): 불필요한 return 제거

seungineer mentioned this pull request Aug 31, 2024

그래프 상에 현재 선택된 pr 정보 명확히 노출 #564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lxxmnmn commented Aug 23, 2024

ytaek left a comment

ytaek Aug 23, 2024

ytaek Aug 23, 2024

seungineer Aug 23, 2024

ytaek Aug 23, 2024

lxxmnmn Aug 23, 2024

seungineer Aug 24, 2024

ytaek Aug 25, 2024

lxxmnmn Aug 25, 2024

ytaek Aug 23, 2024

ytaek Aug 23, 2024

lxxmnmn Aug 23, 2024 •

edited

Loading

ytaek Aug 23, 2024 •

edited

Loading

ytaek Aug 23, 2024

lxxmnmn Aug 23, 2024

seungineer Aug 24, 2024

seungineer left a comment

seungineer Aug 23, 2024

seungineer Aug 23, 2024

seungineer Aug 23, 2024

ytaek left a comment

ytaek Aug 23, 2024

ytaek Aug 23, 2024

lxxmnmn Aug 23, 2024

		@@ -0,0 +1 @@
		export { default as FilteredClusters } from "./FilteredClusters";

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

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

Conversation

lxxmnmn commented Aug 23, 2024

Related issue

Result

Work list

1. 선택된 클러스터 강조

2. 선택된 클러스터 정보 표시

3. 태그 구조 변경

Discussion

ytaek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxxmnmn Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

ytaek Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungineer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytaek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxxmnmn Aug 23, 2024 •

edited

Loading

ytaek Aug 23, 2024 •

edited

Loading