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 컴포넌트 구현 #78

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

jindonyy
Copy link
Contributor

@jindonyy jindonyy commented Nov 1, 2022

close #65


설계

기능 / 형태

  • table -> thead, tbody -> tr -> th, td 순으로 children을 사용한다.
  • th, td에 chlidren을 넣어주면 flex 되어 10px 간격으로 가로로 배치된다.
  • table에 cellWidth를 배열로 지정해주면 colgroup으로 th, td 넓이가 지정된다.
    • 배열에 * 사용 시 나머지 크기를 해당 index의 cell(th, td)이 차지한다.
  • th, td는 항상 한 줄이며 지정된 넓이를 넘을 시 ... 처리된다. (텍스트만)
  • tbody에 있는 tr에 hover시, background-color가 변한다.

접근성

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

고민한 사항

th, td의 width

table에 cellWidth를 배열로 지정해주면 colgroup으로 th, td 넓이가 지정된다.

th, td의 넓이를 지정해주기 위해서는 css를 통해 cell에 직접 넓이를 지정해주거나 html 태그인 colgroup으로 지정해주는 2가지 방법이 있습니다. cell에 직접 지정해줄 경우 th, td 마다 모두 넓이를 지정해줘야하고, colgroup 처럼 * 기능이 없어 유동적으로 늘어나는 cell의 넓이는 calc로 계산해서 지정해줘야하는 불편함이 있어 colgroup으로 지정해주는 방식을 사용했습니다.

ellipsis 처리

th, td는 항상 한 줄이며 지정된 넓이를 넘을 시 ... 처리된다. (텍스트만)

해당 부분을 처리하기 위해 Text 컴포넌트를 td 안에 넣었었지만

  1. FlexBox 안에 Text를 넣을 경우
<td>
  <FlexBox sx={{ ...flexCss, ...sx }}>
    <Text as="div" ellipsis>{children}</Text>
  </FlexBox>
</td>

이 경우, children으로 text와 icon이 들어오는 경우 가로로 배치되지 못합니다.
display: flex는 바로 하위 자식들만 배치하기 때문입니다.

  1. Text 안에 FlexBox를 넣을 경우
<td>
  <Text as="div" ellipsis>
    <FlexBox sx={{ ...flexCss, ...sx }}>{children}</FlexBox>
  </Text>
</td>

이 경우, ... 처리가 되지 않습니다.
때문에 외부에서 ... 처리를 하고 싶은 cell의 경우, 또 폰트를 변경하고 싶은 cell의 경우에 Text를 이용하여 children으로 넣어주는 방식이 낫다고 판단했습니다.

@netlify
Copy link

netlify bot commented Nov 1, 2022

Deploy Preview for co-studo ready!

Name Link
🔨 Latest commit 2883113
🔍 Latest deploy log https://app.netlify.com/sites/co-studo/deploys/6363fdd38064d7000898a7fe
😎 Deploy Preview https://deploy-preview-78--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

@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.

고생하셨습니다 도니~

type 을 파일 상단에 한번에 선언하는 걸 컨벤션으로 지정하는건 어떨까요?

-- type --
-- 외부에 선언되어 사용되어지는 변수 ( css 등 ) ---
-- Component --

Comment on lines 26 to 35
<caption
css={css`
position: absolute;
width: 1px;
height: 1px;
margin: -1px;
overflow: hidden;
clip-path: polygon(0 0, 0 0, 0 0);
`}
>
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 의논해서 알긴 하지만, css 를 별도로 분리해서 변수명을 통해 어떤 의도를 가지고 있는지 설명해줘도 좋겠다는 생각이 들었습니다

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.

저도 동의합니당! ^_^)b
이후에도 위의 코드와 같이 용도가 명확한 스타일들은 따로 변수로 분리해두는게 코드를 이해하기 쉬울 것 같아요!

@@ -53,6 +53,7 @@ module.exports = {
'react/react-in-jsx-scope': 'off',
'react/require-default-props': 'off',
'react/jsx-no-constructed-context-values': 'off',
'react/no-array-index-key': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

array 에 index 를 key 로 써도 되는상황을 대략적으로는 알고있지만,
해당 옵션을 off 한다는 것은 같은 ESLint 를 사용하는 개발자들이 모두 알고있다는 가정을 전제로 하기 때문에 좀 꺼려졌던 것 같아요.

Copy link
Contributor

Choose a reason for hiding this comment

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

파크의 말씀처럼 ESlint 옵션은 모든 개발자가 숙지해야 하는 부분이니 옵션의 변경은 자잘한 옵션이라 해도
discussion이나 스크럼 때 논의를 한번 하고 반영하는게 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 다들 아실거라 생각해서 PR에 적어놓고 댓글로 의견을 받으려고 했는데 깜빡했네여 ^_^);;
좀 꺼려지더라도 현재 제가 사용한 상황처럼 index로만 key를 사용해야할 상황이 생기는데요. 끄지 않는다면 이런 경우에는 어떻게 하면 좋을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

개발 단계에서는 논의 결과를 기다리긴 번거로우니 우선 warn으로 설정해서 개발을 하다가 추후 discussion이나 PR로 논의 한 뒤에 off나 warn으로 확정 짓는 건 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

저는 그냥 간단히 col-${index} 로 해도 괜찮을 듯 한데..

Copy link
Member

Choose a reason for hiding this comment

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

차라리 Col 만 컴포넌트를 분리해서 react v18 의 useId 써도 되지않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

react 공식문서

useId is not for generating keys in a list. Keys should be generated from your data.

라고 적혀있긴 한데.. 사실 데이터가 아닌 경우에는 그냥 써도 되지 않나.. 싶기도 ..🤨

Copy link
Member

Choose a reason for hiding this comment

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

말씀하신 것 처럼 useId 를 쓰는게 의도와 안맞을 수도 있겠네용
eslint 만든 maintainer 가 이 경우에는 override 해서 쓰라는데, 해당 라인에서 disable 시키는 주석을 추가하는걸 얘기하는걸까요?

댓글에 hack 이라고 얘기하는 걸 index 를 key 로 써도 되는 경우 에만 적용해서 쓰도록 하는건? ㅋㅋ

Copy link
Contributor Author

@jindonyy jindonyy Nov 3, 2022

Choose a reason for hiding this comment

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

저렇게 해도 에러 뜨는건 똑같은데.. key 값에 그냥 index를 쓰기만 해도 에러가 떠서,, 하하
제 생각에 저기서 말하는 override는 eslint 규칙을 재정의하라 그 말 같아용.
근데 파크가 말한대로 개발자가 써도 되는 경우를 판단해서 주석으로 끄는거도 괜찮을듯 하네요.
아니면 모르는 개발자도 있을수도 있는게 걱정되는거니 warning 으로 경고만 뜨게 하는거도 괜찮을거 같구요!

+) 생각해보니 warning이 괜찮은거 같아요. 타입스크립트도 any를 쓸 때 경고만 뜨자나요. 이거도 any를 써도 되는 상황 맞아? 라고 경고 하는거니 어쩔수 없이 써야한다고 생각하면 그냥 쓸테고 바꿀 수 있다면 개발자가 경고를 보고 바꾸려고 할테니??

Copy link
Contributor Author

@jindonyy jindonyy Nov 3, 2022

Choose a reason for hiding this comment

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

discussion에 polls로 올려두었으니 의견과 투표 부탁드러요!
@hemudi @healtheloper @mina-gwak

Comment on lines 11 to 17
export interface TableProps extends TableDefaultProps {
caption: string;
cellWidth?: string[];
children: ReactElement | ReactElement[];
}

const Table = ({ caption, cellWidth, children }: TableProps) => (
Copy link
Member

Choose a reason for hiding this comment

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

cellWidth -> columnWidths 로 의견 내봐욥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

width는 복수가 안되는걸로 알아서 columnsWidth로 할께용

Comment on lines 79 to 83
const flexCss = {
justifyContent: 'center',
alignItems: 'center',
gap: '10px',
} as Pick<ComponentProps<typeof FlexBox>, 'sx'>;
Copy link
Member

Choose a reason for hiding this comment

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

px 로 사용하신 이유가 있을까요?

as 로 다운캐스팅 하신 이유도 궁금하네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

px을 사용한건 실수입니다.. ㅎㅎ
다운캐스팅 하지 않으면 아래처럼 사용하려는 곳의 union 중 하나라고 인식하지 못하고 string 타입이라고만 추론되서 사용했습니다!
스크린샷 2022-11-02 오후 6 19 15

Copy link
Member

Choose a reason for hiding this comment

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

타입표명(다운캐스팅) 이 약간 치트라는 느낌을 갖고있었고 그게 왜 그런지는 명확히 잘 모르고 있었는데
같이 참고하실만한 부분 첨부할게요!

Comment on lines 147 to 150
<Table.Th>
미 체크인
<Icon iconName="star" size="large" />
</Table.Th>
Copy link
Member

Choose a reason for hiding this comment

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

전 이 사용법이 좀 어색한 것 같아요.
string 만 들어간다면 Text 를 사용할 수 있게한 점은 좋았는데,
string 이 아니라면 굳이 Text 로 쓰게 해야하나? 라는 생각이 들었어요

만약 Icon 과 같은 컴포넌트도 써야하는 상황이 있다면, 차라리 children 의 타입을 string 과 Row 컴포넌트로 제한해버리는 것도 좋을 것 같다는 생각도 들었어요. (현재 Row 컴포넌트를 조금 수정하긴 해야하지만)

그 두 가지 상황에 맞춰 분기처리도 필요할 것 같고..

<Table.Th>
   <Row content="미 체크인" right={<Icon iconName="star" size="large" />}
</Table.Th>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

우선 Row를 사용하지 않은 이유는 Row에 content가 flex-grow: 1이 지정되어 있어 left, right 넓이를 제외하고 나머지 넓이를 content가 가지게 되는데 제가 사용하는 곳을 flex-grow 속성이 들어가면 안되서 사용하지 못했어요! 사용하려면 Row 컴포넌트에서 flex-grow를 변경할 수 있는 prop을 받게 수정해야하는데 이렇게 되면 그냥 좌우로 배치하는 용으로도 Row를 쓸 수 있게 되고 그럼 그냥 FlexBox를 사용했을 때와 용도가 비슷해지는거 같아요! 제 생각엔 Row의 용도를 좀 더 명확히 해서 네이밍도 변경하는게 좋을거 같단 생각이 드네요. 그리고 초반에 만든 컴포넌트라 안의 내용도 바꿔야할 곳이 많이 보입니다!

th에 Text를 넣어둔건 font-weight: bold를 주기 위해서 였어요! 사실 이거 때문에 ellipsis가 필요한 th에는 두번 Text를 쓰게 되는거라 th의 Text도 빼서 사용자가 ellipsis와 font-weight: bold를 함께 넣어주는게 나을까 고민했습니다.

Comment on lines +47 to +49
{noticeList
.reverse()
.map(({ id, title, writer, date, view, commentCount }) => (
Copy link
Member

Choose a reason for hiding this comment

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

스토리북 코드이긴 하지만 reverse 는 불필요한 작업 같습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

최신 데이터를 맨 위에 보여지기 위해서 사용한건데요. 보내주시는 API가 최신 데이터가 위에 온다면 사용하지 않아도 되는데 오래된 데이터가 먼저 온다면 reverse 하거나 reduceRIght를 써야하는데요. map이 좀 더 간편하고 페이징이니 10개씩 데이터가 올거라 배열이라 reverse해도 성능에 크게 차이가 없다고 생각했어요.

Copy link
Member

Choose a reason for hiding this comment

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

이게 서버가 데이터의 order 를 정리해서 보내줄 수 있는데 오름차순일지 내림차순일지는 모를 수 있고 (제가 만드는거긴 하지만.. ) 아직 명확히 API 를 만들지 않은 상태에서 - 이때는 DB에서 order 가 뒤죽박죽으로 왔었음 - 달았던 리뷰였습니다. 서버가 데이터의 순서를 어떻게 줄지 결정되지 않은 상태에서 reverse 를 왜했지? 라고 생각했나봐요

Comment on lines +103 to +107
const Td = ({ sx = {}, children, ...restProps }: TdProps) => (
<td css={{ padding: '1.7rem 2rem' }} {...restProps}>
<FlexBox sx={{ ...flexCss, ...sx }}>{children}</FlexBox>
</td>
);
Copy link
Member

Choose a reason for hiding this comment

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

FlexBox SX 에 padding 을 추가하고, as='td' 로 대체해도 괜찮을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다시 생각해보니 FlexBox에는 padding을 추가할 수 없네요..^_ㅠ

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.

FlexBox의 sx에는 padding 속성이 없고, css prop은 안넘겨지니 padding을 추가하면 styled를 이용해야 하는데요. 저희 그때 headless 컴포넌트만 styled로 extends 해서 쓰기로 했던거 아닌가요??
제가 말한 css prop을 안받으면 생기는 중첩 문제가 이런 부분이예요ㅠㅠ

Comment on lines 109 to 113
Table.Thead = Thead;
Table.Tbody = Tbody;
Table.Tr = Tr;
Table.Th = Th;
Table.Td = Td;
Copy link
Member

Choose a reason for hiding this comment

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

Table 에 Compound 형식이기 때문에 Thead -> T 라는 의미가 중복되는 느낌이에요

Table.Head
Table.Body
Table.Row
Table.HeadCell
Table.BodyCell

은 어떠신가요?

Comment on lines +91 to +97
const Th = ({ sx = {}, children, ...restProps }: ThProps) => (
<th scope="col" css={{ padding: '1.7rem 2rem' }} {...restProps}>
<Text as="div" sx={{ fontWeight: 'bold' }}>
<FlexBox sx={{ ...flexCss, ...sx }}>{children}</FlexBox>
</Text>
</th>
);
Copy link
Member

Choose a reason for hiding this comment

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

도니가 예시를 들어주신 것 처럼 string + Icon Component 라면 이것도 Text 의 chdilren 으로 들어가는게 맞나? 라는 생각이 들었어요

물론 Text 의 chdilren 으로 다양한 컴포넌트를 받아서, 그 컴포넌트에서는 모두 해당 Text 의 css 를 inherit 받는 식으로 쓸 수도 있겠지만, Text 라는 이름에는 어색한 느낌이 저는 드네요, 차라리 string 으로 제한해버리는 것도 좋지 않을까? 라는 생각입니다.

Copy link
Contributor Author

@jindonyy jindonyy Nov 2, 2022

Choose a reason for hiding this comment

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

string으로 제한해버리면 Text.Hightlight를 사용하지 못하고, Text가 p태그일 때 레이아웃을 위해 span같은걸 사용할 경우도 있잖아요. font 제어가 Text로 밖에 안되는데 string으로 제한할 수는 없을거 같아요!

지금 코드도 위에 적어놨듯이 th는 항상 bold 체여야 하는데 Text를 통해서만 사용할 수 있으니.. 일단 문자는 FlexBox의 children으로 꼭 들어와야하기 때문에 사용하는 곳에서 그 문자를 Text로 감싼 뒤 bold를 적용해서 보내거나 기본 기능으로 제공하고 싶으면 FlexBox 상위에 감싸는 방법밖에 없다고 생각했어요!

Comment on lines 175 to 177
<Table.Td>
<Text ellipsis>{props.dateOfSubscription}</Text>
</Table.Td>
Copy link
Member

Choose a reason for hiding this comment

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

위에서 Th 를 <Table.Th>닉네임</Table.Th> 로 사용하신 것 처럼,

Td 의 Children 으로 Text 를 넣으면 똑같이 동작하지 않을까 기대가 되는데, 여기선 ellipsis 를 위해 Text Component 를 쓴다고 이해가 되는데, 사실 Td 의 기능에 포함될만하다고 생각이 들어서 해당 부분을 추상화해도 될 것 같다는 생각이에요.

<Table.Td>{props.dateOfSubscription}</Table.Td> 이렇게 써도 똑같이 동작하게,

FlexBox 를 as='td' 로 사용해서 Text 컴포넌트를 사용하면 해결되지않을까요?

Copy link
Contributor Author

@jindonyy jindonyy Nov 2, 2022

Choose a reason for hiding this comment

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

이 부분은 PR내용에 적어놨으니 읽어주세요~
th에도 ellipsis 처리하고 싶으면 Text의 children으로 넣어줘야합니다! 사실 다 ellipsis 처리해주는게 맞는데 안에 넣을 수가 없는 구조라.. ellipsis를 th, td 안에서 자동으로 해주고 싶다면 content, right라고 prop을 따로 받아서 내부에서 Text안에 content를 넣고 right는 Text 옆에 넣어주는 방식으로 처리해야할거 같아요.

<FlexBox sx={{ ...flexCss, ...sx }}>
  <Text ellipsis>{content}</Text>{right}
</FlexBox>

Copy link
Member

Choose a reason for hiding this comment

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

ellipsis 을 기본 기능이라고 생각하면 적어주신 방법으로 진행해도 좋을 것 같아요🧐

content 라는 prop 으로 전달한다면 위에서 언급하신 font weight bold 문제도 해결가능하지 않을까요?

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.

th, td 관련해서 의견이 많이 나온 것 같네요. 함께 의논해보고 개선해봐도 좋을 것 같아요!
고생하셨습니다 도니 👍

export interface TableProps extends TableDefaultProps {
caption: string;
cellWidth?: string[];
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.

extends 하는 경우에는 TableProps에서 children을 빼줘도 작동하니까 없애도 되지 않을까요?

</caption>
{cellWidth && (
<colgroup>
{cellWidth?.map((width, index) => (
Copy link
Member

Choose a reason for hiding this comment

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

cellWidth가 존재하는지 위에서 체크해주고 있으니까 cellWidth.map() 이렇게 바로 사용해도 될 것 같아요!

Comment on lines +91 to +97
const Th = ({ sx = {}, children, ...restProps }: ThProps) => (
<th scope="col" css={{ padding: '1.7rem 2rem' }} {...restProps}>
<Text as="div" sx={{ fontWeight: 'bold' }}>
<FlexBox sx={{ ...flexCss, ...sx }}>{children}</FlexBox>
</Text>
</th>
);
Copy link
Member

@mina-gwak mina-gwak Nov 2, 2022

Choose a reason for hiding this comment

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

th, td는 항상 한 줄이며 지정된 넓이를 넘을 시 ... 처리된다. (텍스트만)

저는 이 부분에서 꼭 한 줄로 들어갈 필요가 있을까 싶어요! th, td에 어떤 요소들을 넣었을 때 기본적으로 한줄로 출력되지는 않으니까 사용자가 애초에 그걸 기대하지 않을 수도 있을 것 같아요. (저 같음 그럴 것 같아서요..ㅎㅎ)

만약 member-management-list 스토리에 있는 닉네임 부분에서 유저의 이미지와 닉네임을 세로로 배치하고 싶다고 했을 때, 일반적으로 td를 사용했을 때처럼 td 안에 닉네임과 이미지를 넣어줬을 때 따로 설정해주지 않아도 세로로 배치가 되는 것을 생각했는데 가로로 배치가 되는 상황이니까 다시 컨테이너로 감싸서 넣어줘야 하는 거잖아요. 이런 상황도 있을 수 있다고 생각해요..! 그래서 기본 동작대로 FlexBox로 감싸지 않고 사용하는 쪽에서 적절히 넣어주도록 해주는 건 어떨까 싶습니당

스토리 예시

Copy link
Member

Choose a reason for hiding this comment

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

다시 생각해보니 그렇네요, 차라리 그 칸의 width 를 넘어가지 않도록 font size 지정 및 글자수를 제한하게 하는게 맞을지도??
제공해주는 입장이 결정하는 것이긴 할텐데, 테이블셀에서 생략시키는게 기본기능으로 제공인건 좀 어색할 수 있겠네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

닉네임 부분에서 유저의 이미지와 닉네임을 세로로 배치하고 싶다고 했을 때

세로로 배치를 왜 해야할까요..? 현재 저희 디자인의 Table에서는 항상 가로로만 배치되서 기본 동작으로 넣었습니다!

th, td에 어떤 요소들을 넣었을 때 기본적으로 한줄로 출력되지는 않으니까 사용자가 애초에 그걸 기대하지 않을 수도 있을 것 같아요.

스크린샷 2022-11-02 오후 9 36 42

저희 디자인에서도 길어지면 한 줄로 배치되는 거처럼 지금 테이블이 사용되는 곳들이 list 페이지들인데요. list의 경우 항목이 많고 상세 페이지가 따로 있기 때문에 미리보기 식으로 제공하기 때문에 한줄 이상일 경우에는 ...으로 처리하는 경우가 많아요. 특히 제목을 그대로 다 보여준다면 pc에서는 2줄이던게 모바일같은 경우에는 4~5 줄이 되기 때문에 상세 페이지가 따로 있다면 ... 으로 처리하더라구요!

< 참고: 네이버 >
스크린샷 2022-11-02 오후 9 40 47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다시 생각해보니 그렇네요, 차라리 그 칸의 width 를 넘어가지 않도록 font size 지정 및 글자수를 제한하게 하는게 맞을지도?? 제공해주는 입장이 결정하는 것이긴 할텐데, 테이블셀에서 생략시키는게 기본기능으로 제공인건 좀 어색할 수 있겠네요

width에 넘어가지 않도록 글자수를 제한하도록 하려면 width는 기기마다 다르니 기준을 제일 작은 width 기기를 기준으로 잡아야할텐데.. 그럼 글자수가 너무 작을거 같네요. 그리고 레이아웃을 위해서 글자수를 제한하게 UX적으로 좋은거 같진 않아요~!
위에서도 적어놨듯이 저희는 테이블을 list 용으로만 쓰기 때문에 ... 처리가 맞다고 생각합니다. 그리고 지금 코드 상에서는 사실 ... 처리를 밖에서 해주고 있습니다..! 필요한 부분에서만 ... 처리를 하기 위해서..

Copy link
Member

Choose a reason for hiding this comment

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

세로로 배치를 왜 해야할까요..? 현재 저희 디자인의 Table에서는 항상 가로로만 배치되서 기본 동작으로 넣었습니다!

당장 저희 프로젝트에서 그렇게 배치하는 건 아니긴 한데 저희가 Headless 컴포넌트로 구현하는 것들은 저희 프로젝트뿐만 아니라 범용적으로 쓰인다고 가정하고 만드는 게 아니었나요..?! 저는 그런 걸로 알고 있었고 그래서 별도의 레포로 분리도 한 게 아닐까 싶어서 여러 곳에서 쓰일 때라는 가정을 하고 말씀드렸던 거였어요! 말줄임도 같은 맥락으로 구현하는 방식에 따라 다를 수 있으니 기본으로 포함되는 게 아니라 옵션으로 들어가야하지 않나 싶었습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아하..! 저는 Table은 headless 가 아니라고 생각했어요..! 가로로 배치되고 ellipsis 처리는 UI 부분이지 기능이 아니니까요! Row와 비슷한 역할이라고 생각합니다~!

Copy link
Member

Choose a reason for hiding this comment

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

기능이 추가될 여지는 있지 않을까요? 기본 테이블만 사용하는 경우도 있을 수 있는데 다른 페이지에서 테이블에 정렬 기능이 존재하니까 Headless로 봐도 되지 않을까 싶었어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저는 Headless는 스타일이 없고 기능만 있는 컴포넌트라고 생각했어요. 스타일이 있더라도 Slider처럼 기능을 위한 기본 스타일만 있는? 그리고 여기에 스타일을 입혀서 extends 해서 사용해야하는?
그런데 테이블의 ellipsis와 th의 bold, 글자 정렬 등은 모두 기능보다는 UI에 가깝다고 생각했어요. 그럼 UI만 가지고 있는 컴포넌트는 저희 디자인 시스템에 맞추기로 했으니 한정지어서 만들었습니당. 디자인 시스템 컴포넌트에서는 추후에 추가될 여지보다는 현재 나온 페이지들에서 고려해서 만드는 걸로 생각했는데 아닌가요? ^_ㅠ

Copy link
Member

Choose a reason for hiding this comment

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

headless 에 대한 이야기가 좀 더 필요할 것 같아서 월요일 회의때 의논해봐도 좋을 것 같네요

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.

와 코드를 보는데 많은 고민을 하셨다는게 느껴지네요 넘 고생하셨어요 도니 ㅠ-ㅠ)b
접근성까지 고려해주신 부분이 인상깊네요!
컴포넌트 구조와 관련하여 많은 의견이 나온 것 같고 위에서 나온 것 외의 추가적인 의견은 없는 것 같아 우선 comment로만 리뷰 남기겠습니다!
이후에 PR 코드의 변경사항이 발생하면 다시 리뷰 남기겠습니다!

- add offscreen style constant.
- cellWidth -> columnsWidth
- px -> rem
- change Table children component naming
- remove children type in TableProps
@jindonyy jindonyy changed the title Table 컴포넌트 구현 [Dony] Table 컴포넌트 구현 Nov 3, 2022
@jindonyy jindonyy closed this Nov 8, 2022
@jindonyy jindonyy reopened this Nov 8, 2022
@jindonyy jindonyy closed this Nov 21, 2022
@healtheloper healtheloper deleted the feat/Table/#65 branch December 6, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Table 컴포넌트 구현
4 participants