-
Notifications
You must be signed in to change notification settings - Fork 10
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
DataTableの<th />に文字列以外も含められるようにする #139
Conversation
// MEMO: nameを廃止して、headerCellのみにすることも可能 | ||
// 他の破壊的変更に合わせて行うのが良さそう | ||
name: string; |
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.
name
なくても良さそうと判断していますー
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.
名称をいれたいだけのときも ReactNodeで渡さなきゃいけないようになるとやりすぎな気がするので、stringで名称だけ渡すこともできるようになっているといいかなーと思います headerCell: React.ReactNode | string;
みたいな感じで
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.ReactNode
型にはstring
が含まれているという噂が....
@@ -34,7 +34,7 @@ export const SortableHeaderCell: React.FunctionComponent<Props> = ({ | |||
{...rest} | |||
> | |||
<Flex display="flex" alignItems="center"> | |||
<Typography weight="bold" size="md" component="span"> | |||
<Typography weight="bold" size="md" component="div"> |
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.
入れ子ルール違反を避けるべくdivに
width: 100%; | ||
backdrop-filter: blur(2px); | ||
background-color: ${({ theme }) => | ||
hexToRgba(theme.palette.background.default, 0.9)}; | ||
border-top: ${Size.Border.Small} solid ${({ theme }) => theme.palette.divider}; | ||
border-bottom: ${Size.Border.Small} solid | ||
${({ theme }) => theme.palette.divider}; | ||
${({ theme }) => theme.palette.gray.light}; |
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.
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.
LGTMです!
ref: https://cartaholdings.slack.com/archives/CFB92AX5J/p1595910530074100
close: #136
close: #137