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

fix(Table): table background color matches container background #701

Merged
merged 10 commits into from
Jan 29, 2024

Conversation

alexelie120
Copy link
Contributor

Table component background color should inherit from parent container(th, tr, tfoot)

Tester que les différents modes de la table fonctionnent avec le background color du parent.

Copy link

Storybook for this build: https://ds.equisoft.io/pr-701/

@savutsang
Copy link
Contributor

savutsang commented Jan 16, 2024

Argg l'histoire des fixed header me gosse, ca a l'air que la table ne marche pas de base. J'ai regarde le Figma et je crois que le background devrait etre blanche par default (meme lorsque la table est dans un container gris) et que le inherit est l'exception.
@maboilard ?

Je viens de penser a ca, si on a un props a place <Table bgColor="inherit" /> <Table bgColor="#cccccc" />, ca serait plus simple a utiliser aussi.

@alexelie120
Copy link
Contributor Author

Argg l'histoire des fixed header me gosse, ca a l'air que la table ne marche pas de base. J'ai regarde le Figma et je crois que le background devrait etre blanche par default (meme lorsque la table est dans un container gris) et que le inherit est l'exception. @maboilard ?

Je viens de penser a ca, si on a un props a place <Table bgColor="inherit" /> <Table bgColor="#cccccc" />, ca serait plus simple a utiliser aussi.

J'aime l'idée du props. Laisse moi savoir si tu veux que je l'implémente ou t'aime mieux attendre d'autres feedback.

@maboilard
Copy link

Si je comprends bien, la props va permettre d'hériter de la couleur de background sur laquelle la Data Table se retrouve, donc du même coup, s'appliquer sur le header? C'est bien ça? Si oui, ça me semble être une bonne solution. En tant que non-développeur, j'ai de la misère à me positionner mais je vous fait confiance.

@maboilard
Copy link

@savutsang donc si par défaut le bg est blanc, on serait good avec la props?

@savutsang
Copy link
Contributor

savutsang commented Jan 24, 2024

Si je comprends bien, la props va permettre d'hériter de la couleur de background sur laquelle la Data Table se retrouve, donc du même coup, s'appliquer sur le header? C'est bien ça? Si oui, ça me semble être une bonne solution. En tant que non-développeur, j'ai de la misère à me positionner mais je vous fait confiance.

Oui, le probleme c'est si la table est transparente (meme avec inherit, le parent pourrait etre transparent aussi), les sticky headers serait transparent aussi et ca marcherais pas (on voit le contenu du body a travers), ca prend un background couleur absolument, du moins par default pour que la Table de base fonctionne sans bug.

@savutsang
Copy link
Contributor

@savutsang donc si par défaut le bg est blanc, on serait good avec la props?

Oui par default blanc fixerait le probleme.

@maboilard
Copy link

Alright, donc je vote qu'on laisse blanc par défaut dans ce cas!

@pylafleur
Copy link
Contributor

Je ne suis pas un grand fan des props qui modifient une valeur CSS spécifique (un peu comme nos props noMargin). Est-ce que c'est raisonnable que la manière de modifier la couleur soit par le CSS (styled-components ou en passant un className et .className th { background-color: ... })?

@savutsang
Copy link
Contributor

Je ne suis pas un grand fan des props qui modifient une valeur CSS spécifique (un peu comme nos props noMargin). Est-ce que c'est raisonnable que la manière de modifier la couleur soit par le CSS (styled-components ou en passant un className et .className th { background-color: ... })?

Sauf que cette couleur doit etre appliquer sur un couple d'elements, et il y a de forte chance que si le user ne sais pas ce qu'il fait, ou fait des selector avec une grande specificity, ca peut creer des bugs visuels.

Mais tu me donne une idee, ce qu'on pourrait faire cependant c'est de juste mettre le background-color sur la <table>, et les TR et TH vont inherit le background-color de la table. Comme ca c'est simple pour celui qui utilise styled(Table) ou le classname.

@pylafleur
Copy link
Contributor

ce qu'on pourrait faire cependant c'est de juste mettre le background-color sur la <table>

Ouais, ça pourrait fournir une façon facile d'override pour la grande majorité des cas. Et si quelqu'un veut vraiment être funky avec sa table, il pourra quand même se débrouiller avec la cascade CSS.

Ma crainte c'est que si on expose trop de props pour modifier l'apparence, on va perdre notre garantie de cohérence visuelle. Si le DS s'adressait au grand public ça serait probablement différent mais dans notre contexte, je crois que notre approche conservatrice va aider à maintenir un visuel "conforme" dans les applications.

@@ -569,6 +569,7 @@ interface StickyData {
}

const Wrap = styled.div`
background-color: inherit;
Copy link
Contributor

@savutsang savutsang Jan 25, 2024

Choose a reason for hiding this comment

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

Le background-color n'est pas utile ici.

@savutsang
Copy link
Contributor

savutsang commented Jan 25, 2024

Pour le demo Sticky, au horizontal scroll on voit a travers, en ajoutant background-color: inherit sur le tbody, ca me le fixe.

savutsang
savutsang previously approved these changes Jan 25, 2024
Copy link
Contributor

@meriouma meriouma left a comment

Choose a reason for hiding this comment

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

2 petits détails, sinon 👍

@@ -182,6 +183,7 @@ function getRenderedColumns<T extends object>(rowNumbers: boolean, columns: Tabl
}

const StyledTable = styled.table<StyledTableProps>`
background: white;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
background: white;
background-color: ${({ theme }) => theme.greys.white};

Comment on lines 1215 to 1217
const StyleTable = styled(Table<StickyFooterData>)`
background: #a9cad8;
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sors la définition du styled component à l'extérieur de la story. styled-components fait un warning sinon :

The component Styled(Table) with the id of "sc-hbksqY" has been created dynamically.
You may see this warning because you've called styled inside another component.
To resolve this only create new StyledComponents outside of any render method and function component.

Dans le fond c'est que ça recrée un nouveau component complètement à chaque rendu.

@alexelie120 alexelie120 merged commit 6146126 into master Jan 29, 2024
18 checks passed
@alexelie120 alexelie120 deleted the fix/ABF-7052 branch January 29, 2024 20: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.

5 participants