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
Changes from 5 commits
aa224c8
e7fc404
18bab66
ebd6ccd
d08b98a
2883113
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
import { ReactNode, ReactElement, ComponentProps } from 'react'; | ||
import { css } from 'styled-components'; | ||
|
||
import FlexBox from '@components/common/FlexBox'; | ||
import Text from '@components/common/Text'; | ||
|
||
type TableDefaultProps = { | ||
children: ReactElement | ReactElement[]; | ||
}; | ||
|
||
export interface TableProps extends TableDefaultProps { | ||
caption: string; | ||
cellWidth?: string[]; | ||
children: ReactElement | ReactElement[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extends 하는 경우에는 TableProps에서 children을 빼줘도 작동하니까 없애도 되지 않을까요? |
||
} | ||
|
||
const Table = ({ caption, cellWidth, children }: TableProps) => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cellWidth -> columnWidths 로 의견 내봐욥 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. width는 복수가 안되는걸로 알아서 columnsWidth로 할께용 |
||
<table | ||
css={css` | ||
width: 100%; | ||
overflow: hidden; | ||
border-top: ${({ theme }) => `0.1rem solid ${theme.palette.borderLine}`}; | ||
table-layout: fixed; | ||
`} | ||
> | ||
<caption | ||
css={css` | ||
position: absolute; | ||
width: 1px; | ||
height: 1px; | ||
margin: -1px; | ||
overflow: hidden; | ||
clip-path: polygon(0 0, 0 0, 0 0); | ||
`} | ||
> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 의논해서 알긴 하지만, css 를 별도로 분리해서 변수명을 통해 어떤 의도를 가지고 있는지 설명해줘도 좋겠다는 생각이 들었습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 동의합니당. 의도도 좀 더 명확해질 것 같고, 텍스트를 숨기기 위한 용도면 다른 곳에서도 쓰일 수 있으니 분리해두면 좋을 것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 동의합니당! ^_^)b |
||
{caption} | ||
</caption> | ||
{cellWidth && ( | ||
<colgroup> | ||
{cellWidth?.map((width, index) => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cellWidth가 존재하는지 위에서 체크해주고 있으니까 cellWidth.map() 이렇게 바로 사용해도 될 것 같아요! |
||
<col key={index} width={width} /> | ||
))} | ||
</colgroup> | ||
)} | ||
{children} | ||
</table> | ||
); | ||
|
||
const Thead = ({ children }: TableDefaultProps) => <thead>{children}</thead>; | ||
|
||
const Tbody = ({ children }: TableDefaultProps) => ( | ||
<tbody | ||
css={css` | ||
tr:hover { | ||
background: ${({ theme }) => theme.palette.hoverColor}; | ||
} | ||
`} | ||
> | ||
{children} | ||
</tbody> | ||
); | ||
|
||
const Tr = ({ children }: TableDefaultProps) => ( | ||
<tr | ||
css={css` | ||
border-bottom: ${({ theme }) => | ||
`0.1rem solid ${theme.palette.borderLine}`}; | ||
`} | ||
> | ||
{children} | ||
</tr> | ||
); | ||
|
||
type CellSX = { | ||
width?: string; | ||
justifyContent?: 'center' | 'flex-start'; | ||
}; | ||
|
||
const flexCss = { | ||
justifyContent: 'center', | ||
alignItems: 'center', | ||
gap: '10px', | ||
} as Pick<ComponentProps<typeof FlexBox>, 'sx'>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. px 로 사용하신 이유가 있을까요? as 로 다운캐스팅 하신 이유도 궁금하네요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 타입표명(다운캐스팅) 이 약간 치트라는 느낌을 갖고있었고 그게 왜 그런지는 명확히 잘 모르고 있었는데 |
||
|
||
type ThProps = { | ||
colSpan?: number; | ||
sx?: CellSX; | ||
children: ReactNode; | ||
}; | ||
|
||
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> | ||
); | ||
Comment on lines
+80
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 도니가 예시를 들어주신 것 처럼 string + Icon Component 라면 이것도 Text 의 chdilren 으로 들어가는게 맞나? 라는 생각이 들었어요 물론 Text 의 chdilren 으로 다양한 컴포넌트를 받아서, 그 컴포넌트에서는 모두 해당 Text 의 css 를 inherit 받는 식으로 쓸 수도 있겠지만, Text 라는 이름에는 어색한 느낌이 저는 드네요, 차라리 string 으로 제한해버리는 것도 좋지 않을까? 라는 생각입니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
+80
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
저는 이 부분에서 꼭 한 줄로 들어갈 필요가 있을까 싶어요! th, td에 어떤 요소들을 넣었을 때 기본적으로 한줄로 출력되지는 않으니까 사용자가 애초에 그걸 기대하지 않을 수도 있을 것 같아요. (저 같음 그럴 것 같아서요..ㅎㅎ) 만약 member-management-list 스토리에 있는 닉네임 부분에서 유저의 이미지와 닉네임을 세로로 배치하고 싶다고 했을 때, 일반적으로 td를 사용했을 때처럼 td 안에 닉네임과 이미지를 넣어줬을 때 따로 설정해주지 않아도 세로로 배치가 되는 것을 생각했는데 가로로 배치가 되는 상황이니까 다시 컨테이너로 감싸서 넣어줘야 하는 거잖아요. 이런 상황도 있을 수 있다고 생각해요..! 그래서 기본 동작대로 FlexBox로 감싸지 않고 사용하는 쪽에서 적절히 넣어주도록 해주는 건 어떨까 싶습니당 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 다시 생각해보니 그렇네요, 차라리 그 칸의 width 를 넘어가지 않도록 font size 지정 및 글자수를 제한하게 하는게 맞을지도?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
세로로 배치를 왜 해야할까요..? 현재 저희 디자인의 Table에서는 항상 가로로만 배치되서 기본 동작으로 넣었습니다! 저희 디자인에서도 길어지면 한 줄로 배치되는 거처럼 지금 테이블이 사용되는 곳들이 list 페이지들인데요. list의 경우 항목이 많고 상세 페이지가 따로 있기 때문에 미리보기 식으로 제공하기 때문에 한줄 이상일 경우에는 ...으로 처리하는 경우가 많아요. 특히 제목을 그대로 다 보여준다면 pc에서는 2줄이던게 모바일같은 경우에는 4~5 줄이 되기 때문에 상세 페이지가 따로 있다면 ... 으로 처리하더라구요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
width에 넘어가지 않도록 글자수를 제한하도록 하려면 width는 기기마다 다르니 기준을 제일 작은 width 기기를 기준으로 잡아야할텐데.. 그럼 글자수가 너무 작을거 같네요. 그리고 레이아웃을 위해서 글자수를 제한하게 UX적으로 좋은거 같진 않아요~! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
당장 저희 프로젝트에서 그렇게 배치하는 건 아니긴 한데 저희가 Headless 컴포넌트로 구현하는 것들은 저희 프로젝트뿐만 아니라 범용적으로 쓰인다고 가정하고 만드는 게 아니었나요..?! 저는 그런 걸로 알고 있었고 그래서 별도의 레포로 분리도 한 게 아닐까 싶어서 여러 곳에서 쓰일 때라는 가정을 하고 말씀드렸던 거였어요! 말줄임도 같은 맥락으로 구현하는 방식에 따라 다를 수 있으니 기본으로 포함되는 게 아니라 옵션으로 들어가야하지 않나 싶었습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아하..! 저는 Table은 headless 가 아니라고 생각했어요..! 가로로 배치되고 ellipsis 처리는 UI 부분이지 기능이 아니니까요! Row와 비슷한 역할이라고 생각합니다~! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 기능이 추가될 여지는 있지 않을까요? 기본 테이블만 사용하는 경우도 있을 수 있는데 다른 페이지에서 테이블에 정렬 기능이 존재하니까 Headless로 봐도 되지 않을까 싶었어요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 Headless는 스타일이 없고 기능만 있는 컴포넌트라고 생각했어요. 스타일이 있더라도 Slider처럼 기능을 위한 기본 스타일만 있는? 그리고 여기에 스타일을 입혀서 extends 해서 사용해야하는? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. headless 에 대한 이야기가 좀 더 필요할 것 같아서 월요일 회의때 의논해봐도 좋을 것 같네요 |
||
|
||
export interface TdProps extends ThProps { | ||
rowSpan?: number; | ||
} | ||
|
||
const Td = ({ sx = {}, children, ...restProps }: TdProps) => ( | ||
<td css={{ padding: '1.7rem 2rem' }} {...restProps}> | ||
<FlexBox sx={{ ...flexCss, ...sx }}>{children}</FlexBox> | ||
</td> | ||
); | ||
Comment on lines
+92
to
+96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FlexBox SX 에 padding 을 추가하고, as='td' 로 대체해도 괜찮을 것 같아요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 다시 생각해보니 FlexBox에는 padding을 추가할 수 없네요..^_ㅠ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 무슨 문제가 있을까요?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FlexBox의 sx에는 padding 속성이 없고, css prop은 안넘겨지니 padding을 추가하면 styled를 이용해야 하는데요. 저희 그때 headless 컴포넌트만 styled로 extends 해서 쓰기로 했던거 아닌가요?? |
||
|
||
Table.Thead = Thead; | ||
Table.Tbody = Tbody; | ||
Table.Tr = Tr; | ||
Table.Th = Th; | ||
Table.Td = Td; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Table 에 Compound 형식이기 때문에 Thead -> T 라는 의미가 중복되는 느낌이에요 Table.Head 은 어떠신가요? |
||
|
||
export default Table; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
import { ComponentStory, ComponentMeta } from '@storybook/react'; | ||
|
||
import Icon from '@components/common/Icon'; | ||
import Table from '@components/common/Table'; | ||
import Text from '@components/common/Text'; | ||
|
||
export default { | ||
title: 'common/Table', | ||
component: Table, | ||
} as ComponentMeta<typeof Table>; | ||
|
||
const noticeList = [ | ||
{ | ||
id: 1, | ||
title: '스터디 가입전 해당 공지사항을 숙지해주세요.', | ||
writer: '파크', | ||
date: '2022.10.22', | ||
view: 101, | ||
isFixed: true, | ||
isNew: false, | ||
commentCount: 14, | ||
}, | ||
{ | ||
id: 2, | ||
title: '스터디 전체 공지 드립니다.', | ||
writer: '파크크크크크크크크', | ||
date: '1시간 전', | ||
view: 11, | ||
isFixed: true, | ||
isNew: true, | ||
commentCount: 0, | ||
}, | ||
]; | ||
|
||
export const NoticeList: ComponentStory<typeof Table> = () => ( | ||
<Table caption="공지사항" cellWidth={['85px', '*', '120px', '120px', '85px']}> | ||
<Table.Thead> | ||
<Table.Tr> | ||
<Table.Th>번호</Table.Th> | ||
<Table.Th>제목</Table.Th> | ||
<Table.Th>작성자</Table.Th> | ||
<Table.Th>날짜</Table.Th> | ||
<Table.Th>조회수</Table.Th> | ||
</Table.Tr> | ||
</Table.Thead> | ||
<Table.Tbody> | ||
{noticeList | ||
.reverse() | ||
.map(({ id, title, writer, date, view, commentCount }) => ( | ||
Comment on lines
+50
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 스토리북 코드이긴 하지만 reverse 는 불필요한 작업 같습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 최신 데이터를 맨 위에 보여지기 위해서 사용한건데요. 보내주시는 API가 최신 데이터가 위에 온다면 사용하지 않아도 되는데 오래된 데이터가 먼저 온다면 reverse 하거나 reduceRIght를 써야하는데요. map이 좀 더 간편하고 페이징이니 10개씩 데이터가 올거라 배열이라 reverse해도 성능에 크게 차이가 없다고 생각했어요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이게 서버가 데이터의 order 를 정리해서 보내줄 수 있는데 오름차순일지 내림차순일지는 모를 수 있고 (제가 만드는거긴 하지만.. ) 아직 명확히 API 를 만들지 않은 상태에서 - 이때는 DB에서 order 가 뒤죽박죽으로 왔었음 - 달았던 리뷰였습니다. 서버가 데이터의 순서를 어떻게 줄지 결정되지 않은 상태에서 reverse 를 왜했지? 라고 생각했나봐요 |
||
<Table.Tr key={id}> | ||
<Table.Td> | ||
<Text ellipsis>{id}</Text> | ||
</Table.Td> | ||
<Table.Td | ||
sx={{ | ||
justifyContent: 'flex-start', | ||
}} | ||
> | ||
<Text ellipsis>{title}</Text> | ||
{commentCount > 0 && ( | ||
<Text.Highlight | ||
as="b" | ||
sx={{ fontWeight: 'bold', color: 'primary' }} | ||
> | ||
{commentCount} | ||
</Text.Highlight> | ||
)} | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{writer}</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{date}</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{view}</Text> | ||
</Table.Td> | ||
</Table.Tr> | ||
))} | ||
</Table.Tbody> | ||
</Table> | ||
); | ||
|
||
const memberManagementList = [ | ||
{ | ||
id: 1, | ||
nickName: 'Hemdi', | ||
status: 'waiting', | ||
dateOfSubscription: '2022.10.22', | ||
periodOfActivity: 0, | ||
uncheckin: 0, | ||
uncheckout: 0, | ||
penaltyPoint: 0, | ||
}, | ||
{ | ||
id: 2, | ||
nickName: 'Jamie', | ||
status: 'warning', | ||
dateOfSubscription: '2022.10.22', | ||
periodOfActivity: 0, | ||
uncheckin: 0, | ||
uncheckout: 10, | ||
penaltyPoint: 990, | ||
}, | ||
{ | ||
id: 3, | ||
nickName: 'Park', | ||
status: 'member', | ||
dateOfSubscription: '2022.10.22', | ||
periodOfActivity: 0, | ||
uncheckin: 0, | ||
uncheckout: 0, | ||
penaltyPoint: 0, | ||
}, | ||
]; | ||
|
||
const STATUS = { | ||
member: { | ||
text: '활동 중', | ||
color: 'primary', | ||
}, | ||
waiting: { | ||
text: '가입 승인 대기 중', | ||
color: 'fontColor', | ||
}, | ||
warning: { | ||
text: '벌점 초과', | ||
color: 'warning', | ||
}, | ||
danger: { | ||
text: '경고 대상', | ||
color: 'danger', | ||
}, | ||
}; | ||
|
||
export const MemberManagementList: ComponentStory<typeof Table> = () => ( | ||
<Table | ||
caption="스터디 멤버 관리" | ||
cellWidth={['*', '15%', '15%', '13%', '13%', '13%', '13%']} | ||
> | ||
<Table.Thead> | ||
<Table.Tr> | ||
<Table.Th>닉네임</Table.Th> | ||
<Table.Th>상태</Table.Th> | ||
<Table.Th>가입일</Table.Th> | ||
<Table.Th>활동일</Table.Th> | ||
<Table.Th> | ||
미 체크인 | ||
<Icon iconName="star" size="large" /> | ||
</Table.Th> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 전 이 사용법이 좀 어색한 것 같아요. 만약 Icon 과 같은 컴포넌트도 써야하는 상황이 있다면, 차라리 children 의 타입을 string 과 Row 컴포넌트로 제한해버리는 것도 좋을 것 같다는 생각도 들었어요. (현재 Row 컴포넌트를 조금 수정하긴 해야하지만) 그 두 가지 상황에 맞춰 분기처리도 필요할 것 같고.. <Table.Th>
<Row content="미 체크인" right={<Icon iconName="star" size="large" />}
</Table.Th> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 우선 Row를 사용하지 않은 이유는 Row에 content가 th에 Text를 넣어둔건 font-weight: bold를 주기 위해서 였어요! 사실 이거 때문에 ellipsis가 필요한 th에는 두번 Text를 쓰게 되는거라 th의 Text도 빼서 사용자가 ellipsis와 font-weight: bold를 함께 넣어주는게 나을까 고민했습니다. |
||
<Table.Th> | ||
미 체크아웃 | ||
<Icon iconName="star" size="large" /> | ||
</Table.Th> | ||
<Table.Th>벌점</Table.Th> | ||
</Table.Tr> | ||
</Table.Thead> | ||
<Table.Tbody> | ||
{memberManagementList.map((props) => ( | ||
<Table.Tr key={props.id}> | ||
<Table.Td sx={{ justifyContent: 'flex-start' }}> | ||
<Icon iconName="anonymous" size="large" color="borderLine" /> | ||
<Text ellipsis>{props.nickName}</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis> | ||
<Text.Highlight | ||
as="strong" | ||
sx={{ fontWeight: 'bold', color: STATUS[props.status].color }} | ||
> | ||
{STATUS[props.status].text} | ||
</Text.Highlight> | ||
</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{props.dateOfSubscription}</Text> | ||
</Table.Td> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 위에서 Th 를 Td 의 Children 으로 Text 를 넣으면 똑같이 동작하지 않을까 기대가 되는데, 여기선 ellipsis 를 위해 Text Component 를 쓴다고 이해가 되는데, 사실 Td 의 기능에 포함될만하다고 생각이 들어서 해당 부분을 추상화해도 될 것 같다는 생각이에요.
FlexBox 를 as='td' 로 사용해서 Text 컴포넌트를 사용하면 해결되지않을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 PR내용에 적어놨으니 읽어주세요~ <FlexBox sx={{ ...flexCss, ...sx }}>
<Text ellipsis>{content}</Text>{right}
</FlexBox> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ellipsis 을 기본 기능이라고 생각하면 적어주신 방법으로 진행해도 좋을 것 같아요🧐 content 라는 prop 으로 전달한다면 위에서 언급하신 font weight bold 문제도 해결가능하지 않을까요? |
||
<Table.Td> | ||
<Text ellipsis>{props.periodOfActivity}일</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{props.uncheckin}회</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{props.uncheckout}회</Text> | ||
</Table.Td> | ||
<Table.Td> | ||
<Text ellipsis>{props.penaltyPoint}점</Text> | ||
</Table.Td> | ||
</Table.Tr> | ||
))} | ||
</Table.Tbody> | ||
</Table> | ||
); |
There was a problem hiding this comment.
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 를 사용하는 개발자들이 모두 알고있다는 가정을 전제로 하기 때문에 좀 꺼려졌던 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파크의 말씀처럼 ESlint 옵션은 모든 개발자가 숙지해야 하는 부분이니 옵션의 변경은 자잘한 옵션이라 해도
discussion이나 스크럼 때 논의를 한번 하고 반영하는게 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 다들 아실거라 생각해서 PR에 적어놓고 댓글로 의견을 받으려고 했는데 깜빡했네여 ^_^);;
좀 꺼려지더라도 현재 제가 사용한 상황처럼 index로만 key를 사용해야할 상황이 생기는데요. 끄지 않는다면 이런 경우에는 어떻게 하면 좋을까요?
There was a problem hiding this comment.
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으로 확정 짓는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 그냥 간단히
col-${index}
로 해도 괜찮을 듯 한데..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
차라리 Col 만 컴포넌트를 분리해서 react v18 의 useId 써도 되지않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react 공식문서에
라고 적혀있긴 한데.. 사실 데이터가 아닌 경우에는 그냥 써도 되지 않나.. 싶기도 ..🤨
There was a problem hiding this comment.
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 로 써도 되는 경우 에만 적용해서 쓰도록 하는건? ㅋㅋ
There was a problem hiding this comment.
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를 써도 되는 상황 맞아? 라고 경고 하는거니 어쩔수 없이 써야한다고 생각하면 그냥 쓸테고 바꿀 수 있다면 개발자가 경고를 보고 바꾸려고 할테니??
There was a problem hiding this comment.
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