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] headless Table 구현 #95

Closed
wants to merge 16 commits into from
Closed

[Dony] headless Table 구현 #95

wants to merge 16 commits into from

Conversation

jindonyy
Copy link
Contributor

close #84

지난 번에 보낸 Design system 컴포넌트로 구현했던 table 컴포넌트 PR close하고 정렬 기능 추가하여 headless 컴포넌트로 다시 PR 보냅니다!


설계

기능

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

접근성

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


고민한 사항

sortValues prop을 받을 위치

정렬을 위해 사용할 값을 cell에서 각각 받는게 나은지 table에서 name별로 배열을 만들어서 객체로 받는게 사용하기 편할지 고민입니다.

@netlify
Copy link

netlify bot commented Nov 21, 2022

Deploy Preview for co-studo ready!

Name Link
🔨 Latest commit 0c82af9
🔍 Latest deploy log https://app.netlify.com/sites/co-studo/deploys/637bccd0cc5590000892f6b2
😎 Deploy Preview https://deploy-preview-95--co-studo.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.

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.

정렬을 위해 사용할 값을 cell에서 각각 받는게 나은지 table에서 name별로 배열을 만들어서 객체로 받는게 사용하기 편할지 고민입니다.

테이블을 사용할 때 noticeList를 순회하면서 데이터를 넘겨주는 부분과 sortValues에 들어가는 값이 비슷한 부분이 있지 않나 싶어요. cell에서 각각 받게 된다면 어떤 방식으로 사용하게 되는 건지 좀 더 설명해주실 수 있을까요?

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

Comment on lines +50 to +62
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해서 반환해줘도 되지 않을까요?

view: noticeList.map(({ view }) => Number(view)),
};

const tableCSS = css`
Copy link
Member

Choose a reason for hiding this comment

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

따로 빼서 작성하는 스타일의 네이밍도 통일하면 좋을 것 같아요! 현재는 xxStyle 이런 식으로 많이 사용하고 있는데 이 부분에 대해 한번 논의해봐도 좋을 것 같습니다. 그리고 Headless 컴포넌트의 경우 styled를 사용하기로 했던 것 같은데 css prop을 사용하신 이유는 중복해서 작성하지 않고 여러 테이블에 적용하기 위함일까요?

Comment on lines +16 to +20
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가 현재 어떤 것을 기준으로 정렬하고 있는지에 관한 정보니까 이런 부분을 좀 더 명확히 나타낼 수 있는 이름이면 더 좋겠다는 생각도 드네요!

export const Row: ComponentStory<typeof RowTable> = () => (
<RowTable
caption="공지사항"
columnsWidth={['8%', '24%', '24%', '24%', '24%']}
Copy link
Member

Choose a reason for hiding this comment

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

단순한 궁금증인데 컬럼의 퍼센트를 합쳐서 100%가 되지 않아도 상관이 없는 건가요??

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.

전 일단 cos-ui 로 옮겨져야 하는 코드 같아서 이후에 리뷰 할게요~

@jindonyy jindonyy closed this Nov 28, 2022
@healtheloper healtheloper deleted the feat/Table/#84 branch December 6, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[headless] Table 컴포넌트 구현
3 participants