-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(table,storybook): add header grouping to table header #784
Conversation
653bcc6
to
01ff72b
Compare
01ff72b
to
426ac0f
Compare
426ac0f
to
4b66573
Compare
4b66573
to
ff0782a
Compare
Storybook for this build: https://ds.equisoft.io/pr-784/ |
C'est un cas particulier ici car on a pas de Figma ni de Notion par rapport a ca (c'est bizarre sans les lignes verticales). J'imagine on veut demontrer que c'est un truc fesable mais ce n'est pas encore officiellement supporte/documente. Pour moi c'est good. |
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.
J'ajouterais un séparateur entre les groupes de premier niveau (voir exemple ici) afin de bien délimiter l'information lorsqu'il y a des éléments alignés à droite.
J'ai aussi remarqué que la hauter du column header n'est pas la même que les rows mais ça semble être le cas un peu partout. Je vais donc créer un autre ticket pour régler ce bug.
À part ça, c'est all good pour moi! :)
482028d
317f66c
to
25fc4b9
Compare
1d53331
to
0a3cbf8
Compare
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.
C'est good pour moi, bon boulot!
${({ hasRightBorder }) => hasRightBorder && css` | ||
border-right: 1px solid ${({ theme }) => theme.greys.grey}; | ||
`} | ||
|
||
&:before { | ||
border-bottom: 1px solid ${({ theme }) => theme.greys.grey}; |
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.
Je viens de me rendre compte qu'on pas fait la migration vers les design tokens. Est-ce que c'est volontaire?
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.
Henri s'en occupe dans #851
32e81c6
to
6d0ad4c
Compare
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.
Petits détails, sinon LGTM
${({ hasRightBorder }) => hasRightBorder && css` | ||
border-right: 1px solid ${({ theme }) => theme.greys.grey}; | ||
`} | ||
|
||
&:before { | ||
border-bottom: 1px solid ${({ theme }) => theme.greys.grey}; |
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.
Henri s'en occupe dans #851
3ad9723
to
852d87d
Compare
852d87d
to
91918d7
Compare
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.
Je te laisse voir avec Henri l'ordre dans lequel vous voulez merger vos affaires et pour vous assurer que le token est changé pour un nouveau.
Ref: https://equisoft.atlassian.net/browse/DS-933