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

[Dony] Table 컴포넌트 구현 #7

Merged
merged 18 commits into from Dec 1, 2022
Merged

[Dony] Table 컴포넌트 구현 #7

merged 18 commits into from Dec 1, 2022

Conversation

jindonyy
Copy link
Contributor

@jindonyy jindonyy commented Nov 28, 2022

close #6

지난 번에 보낸 Design system 컴포넌트로 구현했던 table 컴포넌트 PR 참고하여 headless로 구현했습니다.

react package는 아직 ColumnTable만 구현하여 RowTable, BasicTable은 추후에 추가하겠습니다!


설계

기능

  • name을 지정한 head cell을 클릭하면 해당 head cell을 기준으로 정렬 된다.
    • 정렬을 위해 지정해야하는 prop: Table - sortValues, head cell - name (sortValues[name]이 존재해야 한다.)
      • name을 지정해주면 head cell 안에 button이 추가된다.
    • 처음 정렬 기준으로 내림차순이다. (현재 프로젝트에서 사용될 기준으로 잡긴 했으나 여유가 된다면 defaultSortDirection prop을 받아도 될듯 하다.)
    • 정렬하고 싶은 head cell을 클릭 할 때마다 data 속성으로 정렬 방향(direction)을 받는다.

접근성

  • caption을 사용하여 table이 어떤 표를 나타내는지 명시
  • th에 scope를 지정하여 제목행이 가로인지 세로인지 명시

UI

head cell

  • 굵은 글씨
    • 모든 테이블 공통이므로 추가
  • 가운데 또는 왼쪽 정렬
    • table 안에섣 cell마다 다르므로 사용하는 곳에서 Text 컴포넌트를 통해서 지정해주어야 한다.
  • 정렬 방식에 따라 icon 회전, color: primary
    • 모든 테이블 공통이므로 추가

고민한 사항

table 모양에 따른 파편화

table의 head scope가 col이냐 row이냐, head가 없냐에 따라 row와 cell의 구현이 달라져 파편화하였습니다.


Reference

SortingTable

@netlify
Copy link

netlify bot commented Nov 28, 2022

Deploy Preview for idyllic-puffpuff-476e23 ready!

Name Link
🔨 Latest commit 66d6c78
🔍 Latest deploy log https://app.netlify.com/sites/idyllic-puffpuff-476e23/deploys/6388650821c1790009c9766e
😎 Deploy Preview https://deploy-preview-7--idyllic-puffpuff-476e23.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mina-gwak
mina-gwak previously approved these changes Nov 29, 2022
Copy link
Member

@mina-gwak mina-gwak left a comment

Choose a reason for hiding this comment

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

정렬 기능까지 포함하는 테이블 구현하기가 쉽지 않으셨을 것 같은데 고생 많으셨습니다 도니!!

Comment on lines 48 to 60
const getSortIndices = () => {
const sortValuesByName = sortValues[name].map((value, index) => ({
value,
index,
}));
sortValuesByName.sort((a, b) => {
if (a.value > b.value) return direction === DIRECTION.ASCENDING ? 1 : -1;
if (a.value < b.value) return direction === DIRECTION.ASCENDING ? -1 : 1;
return 0;
});

return sortValuesByName.map(({ index }) => index);
};
Copy link
Member

Choose a reason for hiding this comment

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

updateSortState를 실행할 때 매번 이 연산을 수행하게 되는데 같은 컬럼을 여러번 클릭하는 경우에는 매번 계산하는 게 불필요할 수 있을 것 같아요! 현재 sortState에 있는 name과 같다면 가지고 있는 sortIndices를 reverse해서 반환해줘도 되지 않을까요?

Comment on lines 14 to 18
export type SortConfig = {
sortValues?: { [key in string]: (string | number)[] };
sortState: SortState;
setSortState: Dispatch<SetStateAction<SortState>>;
};
Copy link
Member

Choose a reason for hiding this comment

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

별 건 아니지만 문득 도니가 사용하신 [key in string]과 [key: string]이 무슨 차이인지 궁금해서 찾아봤었는데요! 제 생각엔 [key: string] 쪽이 좀 더 적절하지 않나 싶었던 것 같아요.
관련해서 정리한 링크입니다!

그리고 sortState가 현재 어떤 것을 기준으로 정렬하고 있는지에 관한 정보니까 이런 부분을 좀 더 명확히 나타낼 수 있는 이름이면 더 좋겠다는 생각도 드네요!

healtheloper
healtheloper previously approved these changes Nov 29, 2022
Copy link
Member

@healtheloper healtheloper left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 도니~

noticeList 를 어떻게 가져와서 정렬해야할지에 대해 고민이 많으셨을 거라고 생각이 들었습니다.

개인적으로 테이블을 구현했을 때에는 noticeList 를 가장 상위 컴포넌트에 두어서 편하게 개발했었는데, 테이블 각 요소에서 사용자가 원하는 동작이 있을 수 있기 때문에(사용자 이름을 누르면 Dropdown 이 뜬다던지..) 지금같은 Compound 형식으로 제공하는게 취지에 맞다고 생각이 들었네요 [ 비슷한 고민이라고 생각하는 글 부분을 첨부할게요 ]

Comment on lines 3 to 32
const noticeList = [
{
id: 1,
title: '스터디 가입전 해당 공지사항을 숙지해주세요.',
writer: '파크',
date: '2022.10.22',
view: 101,
},
{
id: 2,
title: '스터디 전체 공지 드립니다.',
writer: '도니',
date: '2022.09.20',
view: 110,
},
{
id: 3,
title: '스터디 전체 공지 드립니다.',
writer: '햄디',
date: '2022.09.20',
view: 9,
},
{
id: 4,
title: '스터디 전체 공지 드립니다.',
writer: '제이미',
date: '2022.09.22',
view: 9,
},
];
Copy link
Member

Choose a reason for hiding this comment

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

constants 등으로 따로 분리해도 좋을 것 같네요~

Comment on lines 43 to 58
const SortIcon = () => (
<svg
xmlns="http://www.w3.org/2000/svg"
width="17"
height="14"
viewBox="0 0 17 14"
>
<path
id="다각형_3"
data-name="다각형 3"
d="M7.645,1.408a1,1,0,0,1,1.71,0l6.723,11.073A1,1,0,0,1,15.223,14H1.777a1,1,0,0,1-.855-1.519Z"
transform="translate(17 14) rotate(180)"
fill="#007aff"
/>
</svg>
);
Copy link
Member

Choose a reason for hiding this comment

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

이걸 보니 Icon 도 cos-ui 에 추가해야될까? 라는 생각이 들긴 하네요

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 61 to 65
<ColumnTable
caption="공지사항"
columnsWidth={['85px', '*', '120px', '120px', '120px']}
sortValues={sortValues}
>
Copy link
Member

Choose a reason for hiding this comment

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

rem 은 적용이 안되는거겠죠??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 html에 지정하는 width는 px단위 밖에 안되서요ㅠㅠ
흠.. rem으로 지정하면 px로 계산하는 로직을 추가할까요..??

Copy link
Member

Choose a reason for hiding this comment

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

도니 생각에 사용자가 rem, 혹은 다른 단위로 넣고싶을 거라고 느낀다면 ColumnTable 내부에서 여러 단위에 대해 대응해도 좋다고 생각해요

Comment on lines 49 to 57
const sortValuesByName = sortValues[name].map((value, index) => ({
value,
index,
}));
sortValuesByName.sort((a, b) => {
if (a.value > b.value) return direction === DIRECTION.ASCENDING ? 1 : -1;
if (a.value < b.value) return direction === DIRECTION.ASCENDING ? -1 : 1;
return 0;
});
Copy link
Member

Choose a reason for hiding this comment

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

이것 보고 "가" > "나" 등의 연산이 된다는걸 알았네요 ㅋㅋ
코드를 읽고 조금 생각해봤을 때 문자열 비교가 필요하기도 하고, 숫자 비교도 필요해서 둘 다 할 수있는 방식을 사용했다고 생각했어요
별건 아니지만, 어떤 의도였는지도 코드에서 선언적으로 바로 보이면 좋겠다는 생각도 했어요.

검색해보니 localeCompare 라는 것도 있네요

sortValuesByName 은 배열인데 sort~ 라는건 특정 동작을 하는 함수이지 않을까? 라는 생각도 들었습니다. sorted 정도로 넣으면 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

안그래도 이 부분 이름 때문에 고민을 정말 많이 했는데요..😂
sorted는 정렬된 이라는 형용사로 쓰여 정렬 후에 붙여주는게 맞더라구여.
sort는 정렬, 정렬하다 동명사 둘 다 해당이 되어 문법적으로는 맞는데 동사로 헷갈릴 여지가 있어 고민했습니다ㅠ
참고한 라이브러리에서는 sortConfig라고 사용하고 있길래 그냥 사용하였는데 sortingValues 로 바꿔볼게요!

Copy link
Member

Choose a reason for hiding this comment

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

메서드 체이닝으로 바로 사용해서 sorted 로 넣어도 좋을 것 같아요~!

Comment on lines 36 to 39
date: noticeList.map(({ date }) => {
const numberDate = Number(date.split('.').join(''));
return numberDate.toString().length < 8 ? numberDate * 10 : numberDate;
}),
Copy link
Member

Choose a reason for hiding this comment

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

지금 date 형태가 "YYYY.MM.DD" 인데, 표준으로 쓰이는 Date 의 형태를 쓰는게 어떤가요? (저희 서버에서는 new Date().getTime() 한 값과 동일한 형태로 줍니다)

사용자가 원하는 형태로 쓸 수 있다는 유연함을 줄 수있는 방식이라고 생각해서 방식 자체는 좋다고 생각합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getTime으로 주시는거면 그 값 그대로 사용해서 비교하면 되겠네용 ^0^

Comment on lines 44 to 48
type RowProps = {
scope?: 'row';
sortConfig?: SortConfig;
children: (ReactElement | ReactElement[])[];
};
Copy link
Member

Choose a reason for hiding this comment

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

해당 타입이 조금 생소한데, ReactElement | ReactElement[] 가 아니면 생기는 문제가 있을까요?

Copy link
Contributor Author

@jindonyy jindonyy Nov 29, 2022

Choose a reason for hiding this comment

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

스크린샷 2022-11-30 오전 5 07 22

하나씩 넣어준 HeadCell은 ReactElement 이고 map으로 넣어준 BodyCell은 ReactElement[] 이 되다보니 children 배열의 타입을 ReactElement | ReactElement[] 로 지정해주면 에러가 나더라구요ㅠㅠ ReactNode로 지정해주면 나지 않는데 그럼 string 등을 Element가 아닌 타입도 받게 되어 지정하게 됐습니다 ^_ㅠ..

Copy link
Member

Choose a reason for hiding this comment

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

[ ReactElement, ReactElement[] ] 가 되어서 저렇게 된거군요
표현할 방법을 생각해보긴 했는데 마땅한 생각이 안들긴 하네요, Table 에서만 발생할만한 상황같긴해서..

Comment on lines 51 to 52
const Children = React.Children.toArray(children);
const [headCell, bodyCells] = [Children[0], Children.splice(1)];
Copy link
Member

Choose a reason for hiding this comment

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

const [headCell, ...bodyCells] = React.Children.toArray(children); 으로 같은 결과가 나올 것 같은데, 혹시 타입에러가 있는걸까요?

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 54 to 56
return (
<tr {...restProps}>
{cloneElement(headCell as ReactElement, { scope, sortConfig })}
Copy link
Member

Choose a reason for hiding this comment

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

위 타입이 어떻게 되냐에 따라서 다운캐스팅이 필요 없을 수도 있겠어서 표시를 위해 커멘트 남겨요

@jindonyy jindonyy dismissed stale reviews from mina-gwak and healtheloper via 5734bd9 November 29, 2022 22:03
healtheloper
healtheloper previously approved these changes Dec 1, 2022
Copy link
Member

@healtheloper healtheloper left a comment

Choose a reason for hiding this comment

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

수고하셨어요~ 👍

Comment on lines 5 to 15
const getDateForm = (date: number) => {
const dateArray = [
new Date(date).getFullYear(),
new Date(date).getMonth() + 1,
new Date(date).getDate(),
];

return dateArray
.map((number) => (number < 10 ? `0${number}` : number))
.join('.');
};
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

@jindonyy jindonyy Dec 1, 2022

Choose a reason for hiding this comment

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

playground 는 예시를 만들어놓는 패키지라 utils 폴더가 좀 고민되네요..!
단지 예시로 만든 데이터에 쓸 함수인데 util로 빼는게 적절할까요??

Copy link
Member

Choose a reason for hiding this comment

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

helpers 정도로 빼도 좋을 것 같아요

Copy link
Contributor Author

@jindonyy jindonyy Dec 1, 2022

Choose a reason for hiding this comment

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

분리를 해야하나 말아야하나가 고민이였던건데 분리를 해야한다면 용도로는 util이 맞는거 같아요! 다른 플젝에도 쓸 수 있는 함수니까요.
-> 그럼 helpers 안에 컴포넌트 이름 별로 폴더를 만들어서 컴포넌트에 필요한 함수들을 빼는걸로 할께요!

Copy link
Contributor

Choose a reason for hiding this comment

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

@healtheloper 혹시 utils가 아니라 helpers로 분리하는 이유가 뭔지 여쭤봐도 될까요? 나쁘지는 않은 것 같은데 정확히 어떤 차이 때문에 helpers로 구분하는지 궁금해서요!

Copy link
Member

Choose a reason for hiding this comment

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

@hemudi 관점의 차이이긴 할텐데 utils 라고 하면 "YYYY.MM.DD" 뿐만 아니라 다양한 것도 적용할 수 있어야 할 것 같다고 생각했어요, 예를들어 다른 프로젝트에서 "YYYY:MM:DD:SS" 로 쓰고싶은데? 한다면 살을 덧붙여 추가해내는 법도 있긴 하겠지만 당장에는 해당 프로젝트에서 필요로하는 포맷으로 바꾸는 용도이기 때문에 좀 더 프로젝트의 성격이 들어간 느낌이라 helpers 가 좋지않을까 생각했었습니다

Comment on lines 6 to 9
const sortingValues = {
writer: noticeList.map(({ writer }) => writer),
date: noticeList.map(({ date }) => {
const numberDate = Number(date.split('.').join(''));
return numberDate.toString().length < 8 ? numberDate * 10 : numberDate;
}),
date: noticeList.map(({ date }) => date),
view: noticeList.map(({ view }) => Number(view)),
Copy link
Member

Choose a reason for hiding this comment

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

그냥 써도 Number type 일 것 같아요~

Comment on lines +21 to +24
{columnsWidth.map((width, index) => {
const pxWidth = /[0-9.]+rem/.test(width)
? `${parseFloat(width) * 10}px`
: width;
Copy link
Member

Choose a reason for hiding this comment

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

추후 '%, em 등의 단위도 쓸 수 있게 해주세요' 라는 요구사항이 있을 수도 있다고 생각해요
그러면 삼항연산자가 변경에 대응하기가 좋을까? 라는 생각이 들었어요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

%는 사용가능합니다! em은 부모 박스의 css에 따라 상대적으로 정해지는거라 모든 부모들의 font-size를 계산해야하고 em을 width에 잘쓰지 않아서 고려해도 되지 않다고 생각했어요!
width에 자주 쓰는 px, %, rem 3가지가 가능하니 괜찮지않을까 싶습니다!

Comment on lines +62 to +68
.sort((a, b) => {
if (typeof a.value === 'string' && typeof b.value === 'string')
return b.value.localeCompare(a.value);
if (typeof a.value === 'number' && typeof b.value === 'number')
return b.value - a.value;
return 0;
});
Copy link
Member

Choose a reason for hiding this comment

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

localeCompare 에 numeric 이라는 옵션도 있다고 하네요,
a type 과 b type 이 다를일이 없지 않을까요?

Copy link
Contributor Author

@jindonyy jindonyy Dec 1, 2022

Choose a reason for hiding this comment

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

a type 과 b type 이 다를 일은 없는데 a, b의 value 타입이 number | string 이라 둘 다 체크해줘야 타입 에러가 안뜨더라구요!
numeric 옵션은 문자열인 숫자를 비교할 때 쓰는거라 지금 방식 그대로 사용하는게 나을거 같아요~!

Comment on lines +52 to +56
sortingConfig &&
sortingConfig.sortingState.name === name &&
sortingConfig.sortingState.direction
? 'primary'
: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

name 을 비교하는 연산이 없어지면 어떻게 달라지나요?
이전에는 ?. 연산자로 하셨는데 && 로 바꾸신 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sortingState는 정렬하고자하는 기준의 head cell을 클릭했을 때 set 됩니다! 자신이 클릭된 상태일 때만 color가 바껴야하므로 자신이 클릭된 head cell인지 비교하는 문이예요!
&& 로 바꾼건 ?로 비교문이 많아져 && 조건문 하나로 합치는게 가독성이 좋은거 같아 바꿨습니다!

mina-gwak
mina-gwak previously approved these changes Dec 1, 2022
Copy link
Member

@mina-gwak mina-gwak left a comment

Choose a reason for hiding this comment

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

LGTM 👍
추가로 궁금한 부분은 없어서 approve 하겠습니다 고생하셨어요 도니!!

hemudi
hemudi previously approved these changes Dec 1, 2022
Copy link
Contributor

@hemudi hemudi left a comment

Choose a reason for hiding this comment

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

LGTM!!!
넘 고생하셨어요 도니ㅠㅠ!!

Copy link
Member

@healtheloper healtheloper left a comment

Choose a reason for hiding this comment

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

👍

@jindonyy jindonyy merged commit e9d7faf into dev Dec 1, 2022
@jindonyy jindonyy deleted the feat/Table/#6 branch December 1, 2022 08:52
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.

Table 컴포넌트 구현
4 participants