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

[Refactor] 컴포넌트, 변수명 리펙토링 #204

Merged
merged 15 commits into from
Aug 7, 2024

Conversation

rtttr1
Copy link
Contributor

@rtttr1 rtttr1 commented Aug 3, 2024

해당 이슈 번호

closed #195


체크리스트

  • 🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
  • 💯 테스트는 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 불필요한 코드는 제거했나요?
  • ✅ 컨벤션을 지켰나요?
  • 💭 이슈는 등록했나요?
  • 🏷️ 라벨은 등록했나요?
  • 💻 git rebase를 사용했나요?
  • 🙇‍♂️ 리뷰어를 지정했나요?

📌 내가 알게 된 부분


전체적으로 네이밍을 바꾸어 보았습니다!
네이밍에 관련해서 고민하고 찾아본거 아주 간단하게 정리해둔 노션으로 대체하겠습니다

📌 질문할 부분


📌스크린샷 (선택)

Copy link
Contributor

@wuzoo wuzoo left a comment

Choose a reason for hiding this comment

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

wow 정말로 변수명이 너무 깔끔해진 것 같습니다. 보면서 너무 직관적이다라는 생각이 많이 들었어요 !

다만 제가 생각하기에는 특정 컴포넌트 안에 있는 요소들에 대한 스타일 네이밍은, 해당 컴포넌트 네임을 제거해도 될 것 같다는 ? 생각을 해요. 예를 들면 Document 컴포넌트라면 documentContainerStyle이 아닌 containerStyle !!

정말 고생하셨습니다 ㅎㅎ

@@ -2,7 +2,7 @@ import { css } from '@emotion/react';

import { theme } from '@/common/style/theme/theme';

export const containerStyle = (blockSelected: string) =>
export const documentBarContainerStyle = (blockSelected: string) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 이전 네이밍이 더 좋은 것 같아요 !

어차피 DocumentBar.style.ts 이므로 도큐먼트바에 대한 스타일링이 명시되어있으므로, 그냥 container 스타일이라 해도 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

흠 containerStyle로만 하니 온갖 군데에서 선언한 containerStyle이 자동완성에 다떠서 요렇게 바꾸어 봤는데 저도 너무 길다는 느낌이 들긴해서 다시 돌려놓는게 맞는거 같네요!!

Comment on lines +12 to +13
export const selectedStyle = (selectedTabId: string, tabId: string) =>
selectedTabId === tabId
Copy link
Contributor

Choose a reason for hiding this comment

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

👍👍👍

Comment on lines +15 to +21
onClick={() => onTabClick(selectedTabId, 'selected')}
css={[tabStyle('selected'), selectedStyle(selectedTabId, 'selected')]}>
선택한 블록
</Button>
<Button
onClick={() => onTabClick(selectedId, 'total')}
css={[tabStyle('total'), selectedStyle(selectedId, 'total')]}>
onClick={() => onTabClick(selectedTabId, 'total')}
css={[tabStyle('total'), selectedStyle(selectedTabId, 'total')]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

제가 규홍님께 TabsTab을 컴포넌트로 따로 분리해보면 좋을 것 같다고 말씀드렸는데, 일단은 여기서 고칠 수 있다고 생각되는게 html tag 에요 !

제가 HTML문서를 찾아봤는데, "nav는 문서의 부분 중 다른 페이지 혹은 컨텐츠로의 링크를 보여주는 "구획"이다" 라고 하더라고요 !

즉, nav 안에 ul 안에 li가 있는 구조가 정석적인 구조인 것 같아요, 특히나 탭과 같은 컴포넌트를 구현할 때 !

이 때 또 고려해볼 수 있는건 ulli는 시맨틱 태그가 아니기 때문에 적절히 role을 부여할 수 있을 것 같아요.

실제로 WAI-ARIA 가 제공해주는 role중에는 tablisttab이 있기 때문에 이를 각각 ul, li에 부여해준다면 너무너무 좋을 것 같습니다 !

거기다가 더해서 li 태그를 tab을 통해 사용자가 접근할 수 있도록 해준다면 더더욱 완벽! 할거 같아요. 드롭다운 구현 중에 제가 사용했던 것 같은데 참고해보시면 좋을 것 같아요 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오호!! 이거 브렌치 머지하고 다른 브렌치파서 주용님이 알려주신 방향으로 공컴 구현 도전해보겠습니다!!

Comment on lines 25 to 33
<li css={leftSidebarMenuItemStyle} onClick={onClick}>
<PageIndicatorStick isClicked={isClicked} />
<Flex css={clubInfoStyle(isClicked, isExpanded)}>
<img src={logoUrl} alt={`${children?.toString()} icon`} css={clubLogoStyle} />
<Text tag="body4" css={clubNameStyle(isExpanded)}>
{children}
</Text>
</Flex>
</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

이것도 규홍님이 판단하신 것처럼 왼쪽 사이드바의 메뉴 아이템들이니 tab을 통해 접근이 가능하다면 ? 또한 'Enter` 눌렀을 때 이동까지 가능하다면 ? 너무 좋을 것 같네요. 접근성 측면에서 !!

  • aslant disable 줄도 지울 수 있을 것 같습니다. role을 부여한다면요 !

Copy link
Member

@namdaeun namdaeun left a comment

Choose a reason for hiding this comment

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

전체적으로 변수명을 많이 고민해보신게 느껴집니다,,! 코드가 읽기 너무 수월해서 좋네요
저도 규홍님 코드 보면서 변수명에 대해 한 번 더 고민해볼 수 있었어요
고생많으셨습니다 :)

const { isOpen, openModal, closeModal, currentContent } = useModal();

const fileName = children?.toString();
const documentName = children?.toString();
Copy link
Member

Choose a reason for hiding this comment

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

file ->document로 변경해주니 코드 전체적으로 통일감이 느껴져서 좋네요👍

const { isOpen, openModal, closeModal, currentContent } = useModal();

const fileName = children?.toString();
const documentName = children?.toString();

const teamId = useTeamId();

//문서 클릭시 띄워주는 함수
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 Author

Choose a reason for hiding this comment

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

동의합니다!

@@ -19,23 +19,23 @@ import { useTeamId } from '@/shared/store/team';
import { blockNameStyle, deleteBtnStyle } from './SelectedBlock.style';

interface DocumentBarInfoProps {
selectedId: string;
selectedTabId: string;
blockName: string;
selectedBlock: Block;
onClickClose: () => void;
Copy link
Member

Choose a reason for hiding this comment

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

onClose는 어떤가요 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

동의합니다!

Copy link
Member

Choose a reason for hiding this comment

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

요기는 따로 분리하기 보다 LeftSidebarMenuItem에 추가해도 괜찮을 것 같다는 의견입니다
컴포넌트가 너무 잘게 쪼개진 느낌 ? 이에용 어떻게 생각하실까요??

Copy link
Contributor

@Bowoon1216 Bowoon1216 left a comment

Choose a reason for hiding this comment

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

변수명으로 고민하신 게 보이네요!! 상세한 변수명 좋습니다 👍
수고하셨습니다 🤗

Copy link
Contributor

@cindy-chaewon cindy-chaewon left a comment

Choose a reason for hiding this comment

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

컴포넌트 , 변수명 바꾸니까 코드가 훨 읽기 쉬워진거 같아요!! 고생하셨습니당!!

@@ -2,7 +2,7 @@ import { useQuery } from '@tanstack/react-query';

import { getDocuments } from '@/shared/api/time-blocks/team/time-block';

export const useBlockQuery = (teamId: number, blockId: number) => {
export const useBlockInfoQuery = (teamId: number, blockId: number) => {
return useQuery({
queryKey: ['document', blockId],
Copy link
Contributor

Choose a reason for hiding this comment

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

블럭 내용 관련 이라면 쿼리키를 'blockInfo' 와 같은 블록 내용관련으로 바꿔도 좋을 거 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

요거는 문서 삭제되면 전체문서, 블록 문서 두개 모두 재실행 시키기 위해서 두 쿼리의 첫 쿼리키를 document로 지정해 보았습니다!

@rtttr1 rtttr1 merged commit 428a96a into develop Aug 7, 2024
2 checks passed
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