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

Display a chevron on accordion item "preferences" #46

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Badatos
Copy link

@Badatos Badatos commented Jan 28, 2025

…to better see it can be opened.

@Badatos Badatos changed the title Display a chevron on accordion item "preferences" [WIP] Display a chevron on accordion item "preferences" Jan 28, 2025
@floriannari floriannari self-assigned this Jan 28, 2025
@Badatos
Copy link
Author

Badatos commented Jan 28, 2025

Je viens de me rendre compte que selon le navigateur, il y avait déjà un chevron (ou pas).
Avec cette modif, il y a donc maintenant 2 chevrons sur les navigateurs où il y en avait déjà un.
Je continue à regarder pour qu'il n'y en ai plus qu'un sur tous les navigateurs.

@floriannari
Copy link
Contributor

C'est une bonne idée !
C'est vrai que si on ne le sait pas, visuellement rien n'indique qu'on peut cliquer pour dérouler.

Plutôt que d'ajouter une image et de la manipuler en CSS. Je propose de plutôt mettre simplement display: list-item; sur les <summary>

@floriannari
Copy link
Contributor

avec display: list-item; :
image
image

…e it on all browsers, and replace by chevron icon

+ Prevent a multi-line menu to be displayed over the next item.
@Badatos
Copy link
Author

Badatos commented Jan 28, 2025

Je viens de pousser une correction qui rend l'affichage uniforme sur tous les navigateurs (testé sur Firefox, Safari et Chrome)
J'ai conservé l’icône qui est plus uniforme, car la solution du display: list-item ne fonctionne pas sur Safari. Et vu que le style par défaut des summary est écrasé par materializecss, ce sera plus consistant avec les futures mises à jour de la librairie.

@floriannari
Copy link
Contributor

floriannari commented Jan 28, 2025

Je viens de faire le test avec Safari (sur IOS).
En fait même actuellement le chevron s'affiche (et donc display: list-item; sera inutile et ne changera rien)

@floriannari
Copy link
Contributor

floriannari commented Jan 28, 2025

Je viens de tester sur le mac de mon collègue.
Sur Safari (sur mac) ça fonctionne comme avec les autres navigateurs (donc pas de chevron actuellement, mais avec un chevron en cas de display: list-item;)

@floriannari
Copy link
Contributor

Et vu que le style par défaut des summary est écrasé par materializecss, ce sera plus consistant avec les futures mises à jour de la librairie.

Concernant la potentielle mise à jour de materialize, c'est un vrai chantier, et à priori c'est pas quelque-chose de prévu tout de suite.
Actuellement on est en 0.97.8.
Mais il y a eu plusieurs mises à jours cassantes jusqu'en 1.0.0.
Ensuite Materialize est mort, puis a été repris par un fork 4 ans plus tard (fork qui a lui aussi fait plein de mises à jour cassantes) https://github.com/materializecss/materialize

Je pense que quand on fera la mise à jour, on se posera sérieusement la question de migrer directement vers un autre framework CSS

@Badatos Badatos changed the title [WIP] Display a chevron on accordion item "preferences" Display a chevron on accordion item "preferences" Jan 28, 2025
@Badatos
Copy link
Author

Badatos commented Jan 29, 2025

Va pour le "display: list-item" alors. Du moment que Safari ne décide pas un jour d'ajouter une puce devant le chevron, ca me va ;)
Le plus important pour moi est qu'il y ait un indice visuel. Je modifie la PR en ce sens.

@Badatos Badatos changed the title Display a chevron on accordion item "preferences" [WIP] Display a chevron on accordion item "preferences" Jan 29, 2025
@Badatos Badatos marked this pull request as draft January 29, 2025 08:19
@floriannari
Copy link
Contributor

D'ailleurs, pour la langue, on pourrait ajouter 'langue" devant la planète
(Et ça rendrait mieux que juste la puce devant une image)

@Badatos
Copy link
Author

Badatos commented Jan 29, 2025

D'ailleurs, pour la langue, on pourrait ajouter 'langue" devant la planète (Et ça rendrait mieux que juste la puce devant une image)

C'est fait dans mon autre PR #45 , mais bien vu en effet ^^

@Badatos Badatos marked this pull request as ready for review January 29, 2025 09:18
@Badatos Badatos changed the title [WIP] Display a chevron on accordion item "preferences" Display a chevron on accordion item "preferences" Jan 29, 2025
@Badatos
Copy link
Author

Badatos commented Jan 29, 2025

Voila qui est fait. (testé fonctionnel sur Firefox, Chrome et Safari)

@floriannari
Copy link
Contributor

Bonjour et désolé pour le délai.

Avec l'ajout du chevron, je trouve que cela rend moins bien chevron + planète côte-à-côte.
image

ça vous convient si je fais chevron + langue + planète ?
image

@Badatos
Copy link
Author

Badatos commented Feb 17, 2025

L'important, c'est :

  • qu'il y ait le texte en plus de l'icone
  • Qu'il y ai un chevron indiquant que c'est un élément pliable
  • Que l'ergonomie soit uniforme avec les autres menus

.side-nav ul li ul li {
height: 48px;
.side-nav a {
/* Prevent a multi-line menu to be displayed over the next item. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Sans line-height: normal;, une ligne qui dépasse prend la place de 2 items (par exemple webauthn en Anglais)
image

Ensuite sans align-content: center;, le premier item de chaque liste se retrouve collé en haut
image

Remettre ces 2 règles corrige le problème
image

@floriannari
Copy link
Contributor

floriannari commented Feb 20, 2025

Je peux remettre ces 2 règles sans que ça n'altère l'accessibilité et les bonnes pratiques promues par vos commits ?

@Badatos
Copy link
Author

Badatos commented Feb 20, 2025

Je peux remettre ces 2 règles sans que ça n'altère l'accessibilité et les bonnes pratiques promues par vos commits ?

oui, à priori pas de souci ^^. Dans mon souvenir, la correction était pour éviter qu'un element sensé etre sur 2 lignes soit à moitié caché (à cause de la hauteur fixe).
Si tout s'affiche et qu'on arrive à mieux distinguer la séparation entre chaque élément, c'est encore mieux 👍

@Badatos
Copy link
Author

Badatos commented Feb 20, 2025

J'ai remis en place ces 2 règles CSS pour faciliter la fusion.

@floriannari
Copy link
Contributor

(vu que les commits sont un peu désorganisés, je vais squash les PR ensemble)

floriannari pushed a commit that referenced this pull request Feb 20, 2025
This is a squash of multiples PR from Badatos

these PR improve :
- accessibility,
- compliance with typographical rules,
- formatting of certain files (e.g. removal of unnecessary spaces)

#42
Improve OTP-Manager general layout accessibility:

* Put "lang" value in "/" route
* Replace #app.container div by main for more explicit client layout
* Replace H2 by H1 in index.jade and H5 by P
* put lang attribute in html tag for general layout

#44
improved readme presentation
#45
* Increase duration of all toasted errors
* Replace some "b" html tags by "strongs"
* Use non breakable spaces in french strings punctuations
* Remove unwanted spaces in English strings (no spaces before punctuations in english)
* Display textual "language" on language icon
* CSS : add styles for heading (h1,2,3...)
* page-title now use H1 instead of p
* Correct some heading hierarchy
* Remove every `text-align: justify` (Avoid Justify align in every web pages for Accessibility)

#46
Display a disclosure triangle on accordion item "preferences", to better see it can be opened
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.

2 participants