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

New comp Alert #2175

Merged
merged 3 commits into from
Jul 27, 2022
Merged

New comp Alert #2175

merged 3 commits into from
Jul 27, 2022

Conversation

JF-Cozy
Copy link
Collaborator

@JF-Cozy JF-Cozy commented Jun 24, 2022

demo :

On va pouvoir déprécier Alerter, Infos et Banner... dans un second temps, quand on aura un peu éprouvé ces 2 nouveaux composants

validé par @joel-costa

@bundlemon
Copy link

bundlemon bot commented Jun 24, 2022

BundleMon

Unchanged files (2)
Status Path Size Limits
dist/cozy-ui.min.css
19.62KB +5%
dist/cozy-ui.utils.min.css
9.97KB +5%

No change in files bundle size

Groups updated (1)
Status Path Size Limits
transpiled/react/**
499.71KB (+2.05KB +0.41%) -

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@Crash--
Copy link
Contributor

Crash-- commented Jun 24, 2022

Fais-moi rêver, on va modifier le Alerter pour qu'il affiche une Alert? Aussi quid de https://docs.cozy.io/cozy-ui/react/#/Content?id=infos ou encore https://docs.cozy.io/cozy-ui/react/#/Basics?id=banner ?

@Ldoppea
Copy link
Member

Ldoppea commented Jun 27, 2022

Nice :)

This PR is WIP, so no review expected yet?

@JF-Cozy
Copy link
Collaborator Author

JF-Cozy commented Jun 28, 2022

Fais-moi rêver, on va modifier le Alerter pour qu'il affiche une Alert?

Modifier le Alerter je sais pas, mais en tout cas il est question ensuite d'avoir un snackbar basé sur Alert oui. J'ai pas encore poussé la réflexion jusqu'à l'Alerter mais à vu de nez comme ça, ce sera surement un nouveau composant qui va déprécier Alerter.

J'ai conscience qu'il y a aussi Infos et Banner qui sont proches, ces composants seront gérés d'une manière ou d'une autre (je sais pas encore comment) quand la PR sortira de draft

This PR is WIP, so no review expected yet?

Non, il faut également d'abord que @joel-costa valide l'aspect visuel

@JF-Cozy JF-Cozy force-pushed the feat/alert branch 4 times, most recently from 4ff25ec to 07f0717 Compare July 1, 2022 14:29
@joel-costa
Copy link
Contributor

joel-costa commented Jul 4, 2022

  • Le titre doit être en gras
  • Action inline : on a un bouton qui se retrouve sur 2 lignes
  • Est-ce qu’on peut passer la couleur du boutton close en secondary ?
  • On peut observer une variation des padding-top et bottom au passage des boutons en block/inline. Si on peut unifier ça c’est top
  • ❌ Sur la variante “color”, le texte et les icones doivent passer en blanc (le nom de la variante ne me semble pas ouf, peut-être un truc genre “opaque color” ou “full color” ?)
  • La variante "icon" est étrange. Tu l’as mise pour coller avec le composent ”banner” ? Moi ça me va bien mais il faudrait à mon sens aussi ajouter une variante “noIcon”, non ?
  • Le container des boutons a un margin-right de -8px pour compenser le padding des text-buttons. Sauf qu’ici on a des “buttons” en small et donc avec 6px de padding, si on peut passer le container à -6px au lieu de -8px c’est top :)

Info : Pour moi le composent “info” est voué à disparaitre, remplacé par “alert”. Par contre on a “InfoCaroussel” basé sur “Info” qui gère l’affichage de plusieurs “infos” et qui ne peut pas être remplassable aujourd’hui par “alert”. Est-ce qu’on devrait ajouter une variante à “alert” qui gère du multi affichage ? Ou créer un “alertCaroussel” ?

Alerter : Pour moi le composent “alerter” devra disparaitre et être remplacé par “snackbar”, qui utilise “alert” si je ne dis pas de bêtises ?

Banner : Pour moi on a 2 choix :

  • On supprime banner qu’on remplace par “alert” et on lui ajoute une variante “banner” ou “full width”
  • On garde notre “banner” actuelle. qui coexiste avec “alert”. Est-ce que notre “banner” actuelle est basée sur le “alert” de MUI ? Est-ce qu’il serait pertinent de plutôt la baser sur le “alert” de Cozy UI ?

@JF-Cozy
Copy link
Collaborator Author

JF-Cozy commented Jul 4, 2022

@joel-costa pour Banner à voir ensemble, tu peux triturer la doc pour te faire une idée, mais il est possible d'avoir environ le même rendu avec l'Alert actuel.

Par contre tu parles de fullwidth... l'Alert est fullwidth là, c'est pas censé être le cas ??

@JF-Cozy
Copy link
Collaborator Author

JF-Cozy commented Jul 8, 2022

Sur la variante “color”, le texte et les icones doivent passer en blanc (le nom de la variante ne me semble pas ouf, peut-être un truc genre “opaque color” ou “full color” ?)

Color n'est pas une variante, c'est un exemple de la doc pour montrer qu'on peut forcer la couleur de fond si nécessaire. Attention les "variant selector" de la doc ne sont pas les variant possible sur le composant.

La variante "icon" est étrange. Tu l’as mise pour coller avec le composent ”banner” ? Moi ça me va bien mais il faudrait à mon sens aussi ajouter une variante “noIcon”, non ?

Oui exactement, c'est un exemple de doc avec une icone plus grosse.

Pourquoi une variante noIcon ? On prévoit d'avoir un Alert sans icone ? Pour l'instant ce n'est pas possible il y a toujours une icone par défaut.

Action inline : on a un bouton qui se retrouve sur 2 lignes

Dans quel cas précisément ? Sur Mobile avec beaucoup de texte et des longs boutons ? On a le comportement de Mui, donc j'imagine qu'il faut éviter ce cas de figure tout simplement et passer en "block" dans ce cas... 🤔

On peut observer une variation des padding-top et bottom au passage des boutons en block/inline. Si on peut unifier ça c’est top

Non pas sur les boutons, mais sur le container principal oui. Car c'est le cas des maquettes...

Par contre on a “InfoCaroussel” basé sur “Info” qui gère l’affichage de plusieurs “infos” et qui ne peut pas être remplassable aujourd’hui par “alert”. Est-ce qu’on devrait ajouter une variante à “alert” qui gère du multi affichage ? Ou créer un “alertCaroussel” ?

Je suis d'avis de mettre le InfoCaroussel de côté, on verra plus tard...

@JF-Cozy JF-Cozy force-pushed the feat/alert branch 3 times, most recently from 45c9c0e to 6c8b520 Compare July 8, 2022 13:28
@JF-Cozy
Copy link
Collaborator Author

JF-Cozy commented Jul 8, 2022

@joel-costa nouvelle version, normalement tout est bon concernant les précédentes remarques. J'ai ajouté des exemples dans la démo pour les filled et outlined.

Dernier souci : les boutons dans les actions d'un alert filled. Comme on utilise des boutons text, ils étaient couleur sur couleur, donc vert sur fond vert. J'ai feinté en utilisant alors des boutons primary plutôt que text, car on veut ici un bouton blanc. Ca marche plutôt bien, sauf pour le filled secondary... si t'as une idée ?

@joel-costa
Copy link
Contributor

@joel-costa nouvelle version, normalement tout est bon concernant les précédentes remarques. J'ai ajouté des exemples dans la démo pour les filled et outlined.

Dernier souci : les boutons dans les actions d'un alert filled. Comme on utilise des boutons text, ils étaient couleur sur couleur, donc vert sur fond vert. J'ai feinté en utilisant alors des boutons primary plutôt que text, car on veut ici un bouton blanc. Ca marche plutôt bien, sauf pour le filled secondary... si t'as une idée ?

T'as essayé d'ajouter des boutons sur les snackbars customisés de MUI pour voir s'ils géraient le cas ? https://mui.com/material-ui/react-snackbar/#customization

@JF-Cozy
Copy link
Collaborator Author

JF-Cozy commented Jul 25, 2022

Attention le lien est https://v4.mui.com/components/snackbars/#customized-snackbars (v4)

Je viens de tester, part défaut ça fait un bouton noir :

image

sinon en color="inherit" sur le bouton on a un bouton blanc, comme le texte

image

C'est un problème de nos boutons (#2194) et non de l'Alert, je pense qu'on peut passer outre ici afin d'avancer le sujet, d'autant plus qu'on peut quand même avoir un rendu convenable à mon sens en mettant la même couleur sur le bouton que sur l'alerte.

@JF-Cozy JF-Cozy marked this pull request as ready for review July 26, 2022 14:17
Copy link
Member

@Merkur39 Merkur39 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 (si l'histoire des boutons est réglée avec @joel-costa )

react/Alert/index.jsx Show resolved Hide resolved
react/Alert/index.jsx Show resolved Hide resolved
react/Snackbar/index.js Show resolved Hide resolved
react/Alert/index.jsx Show resolved Hide resolved
@JF-Cozy JF-Cozy merged commit 20b89e4 into master Jul 27, 2022
@JF-Cozy JF-Cozy deleted the feat/alert branch July 27, 2022 09:31
@cozy-bot
Copy link

🎉 This PR is included in version 70.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants