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

Force affichage message si cible ancre #5949

Merged
merged 1 commit into from
Jul 9, 2022

Conversation

viki53
Copy link
Contributor

@viki53 viki53 commented Oct 6, 2020

Corrige le troisième point de ce sujet.

Actuellement si un message caché est la cible d'un lien avec une ancre (par exemple lorsqu'on suit une notification), l'ancre ne mène à rien et le message est caché.

Cette PR corrige cela en forçant l'affichage du message en question si l'ancre de la page le cible directement.

Contrôle qualité

  • Lancez make build-front ;
  • Suivez un lien vers un message caché (idéalement vraiment caché, avec un message msg-are-hidden qui va bien) ;
  • Le message doit être accessible et l'ancre doit fonctionner.

À noter que j'ai dû déplacer l'id qui sert pour l'ancre, cela peut risque de casser autre chose, par exemple si du JS l'utilise (ce qui a priori ne devrait pas être le cas).

@AmauryCarrade AmauryCarrade added C-Front Concerne l'interface du site S-Évolution Ajoute de nouvelles fonctionnalités labels Oct 6, 2020
AmauryCarrade
AmauryCarrade previously approved these changes Oct 6, 2020
Copy link
Member

@AmauryCarrade AmauryCarrade left a comment

Choose a reason for hiding this comment

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

✔️ QA OK

Par contre ça a l'effet secondaire de rendre le lien au dessus en apparence inopérant (du moins s'il n'y a qu'un seul message masqué dans cette série) tant que le message a le focus (cliquer dessus ne masque pas le message).

Le lien inopérant

À voir si c'est gênant.

@viki53
Copy link
Contributor Author

viki53 commented Oct 6, 2020

Ah oui j'avais oublié ça…

Après quand on clique sur le lien de base il reste présent pareil une fois le message affiché. Ça change au final pas tant que ça le comportement, à part que ça ne cache pas le message vu que l'ancre reste active (ce qui pourrait se corriger dans le JS du lien/bouton en question)

@AmauryCarrade
Copy link
Member

Effectivement. C'est d'ailleurs une question que je m'étais posée pour l'implémentation de #5039, où l'emphase serait également persistante tant que l'ancre n'est pas retirée.

On peut remarquer que GitHub gère ça en retirant l'ancre de l'URL quand on clique hors du message ciblé. On pourrait peut-être s'inspirer d'une solution comme cela.

@coveralls
Copy link

coveralls commented Oct 6, 2020

Coverage Status

Coverage remained the same at 88.033% when pulling 670559f on viki53:afficher-message-cache-ancre into b9e68a8 on zestedesavoir:dev.

@viki53
Copy link
Contributor Author

viki53 commented Oct 6, 2020

On peut. Faudra que je regarde le JS du lien/bouton du coup, et je mettrai quelques lignes pour retirer l'ancre de l'URL sans casser le scroll.

@AmauryCarrade
Copy link
Member

Stocker le défilement du body avant de supprimer l'ancre et le restaurer devrait suffire, a priori ?

@viki53
Copy link
Contributor Author

viki53 commented Oct 6, 2020

Oui c'est ce que je pensais faire, mais il y a une API changer l'ancre/URL de façon transparente directement de mémoire, ce serait plus simple/propre.

@AmauryCarrade
Copy link
Member

Si ça existe, alors oui, partons là dessus. 👍

@viki53
Copy link
Contributor Author

viki53 commented Oct 6, 2020

Bon ça a l'air jouable, mais :target n'est pas recalculé…

C'est un comportement connu des navigateurs : https://bugs.webkit.org/show_bug.cgi?id=83490

Y'aurait un fix, mais je trouve ça lourd/sale pour pas grand chose : whatwg/html#639 (comment)

@AmauryCarrade
Copy link
Member

AmauryCarrade commented Oct 6, 2020

À voir, parce qu'on va avoir besoin de quelque chose comme ça quand les messages ciblés auront une aura, de toute façon. Donc si c'est pas maintenant (dans une fonction à part réutilisable idéalement)… ce sera plus tard.

(Je ne pensais pas que tu parlais de l'API d'historique initialement, j'imaginais qu'il y avait une API quelconque pour retirer l'ancre sans déplacer la vue)

@viki53
Copy link
Contributor Author

viki53 commented Oct 12, 2020

@AmauryCarrade J'ai poussé ma version, à tester quand même on sait jamais, c'est peut-être ma version de Firefox qui délire.

@AmauryCarrade
Copy link
Member

Il faudra reprendre ici une fois #5960 fusionnée.

@viki53
Copy link
Contributor Author

viki53 commented Mar 10, 2021

Est-ce que c'est toujours d'actualité ?

Y'a des conflits de merge mais ça devrait pas être catastrophique

@Arnaud-D
Copy link
Contributor

Arnaud-D commented Jun 6, 2021

On fait quoi ici ? Il suffit de rebase en résolvant les conflits ?

@viki53
Copy link
Contributor Author

viki53 commented Jun 8, 2021

Il faudrait, mais j'ai l'impression que ça a pas mal dévié entre les deux branches… faudrait reprendre ça en local pour tester

@Arnaud-D
Copy link
Contributor

@viki53 tu as toujours envie de terminer cette PR (si ça te rappelle encore quelque chose :D) ? C'est une fonctionnalité pratique, ça serait dommage que nous ne fassions pas aboutir le développement si près du but !

@viki53
Copy link
Contributor Author

viki53 commented Jun 16, 2022

Alors oui… faut "juste" que je me motive à rebase pour m'assurer que rien ne soit cassé :D

@viki53 viki53 force-pushed the afficher-message-cache-ancre branch 2 times, most recently from a4912ac to 12be18b Compare July 7, 2022 15:32
@viki53
Copy link
Contributor Author

viki53 commented Jul 7, 2022

J'ai rebase en local, faut que je relance le projet pour tester que c'est toujours fonctionnel mais "en théorie" c'est bon

@viki53 viki53 force-pushed the afficher-message-cache-ancre branch from 12be18b to 670559f Compare July 7, 2022 16:32
@viki53
Copy link
Contributor Author

viki53 commented Jul 7, 2022

C'est confirmé en local, c'est bon 👍

Le seul "problème" c'est le bloc "Un message a été masqué" qui reste apparent mais c'est plus compliqué à faire avec juste du CSS (peut-être voir si un sélecteur du style :has(~ .topic-message:target) pourrait aider)…

Copy link
Contributor

@Arnaud-D Arnaud-D left a comment

Choose a reason for hiding this comment

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

QA déjà faite auparavant et c'était OK. Et bien sûr j'ai rejeté un œil rapidement. ^^

@Arnaud-D Arnaud-D merged commit 8bcd875 into zestedesavoir:dev Jul 9, 2022
@viki53 viki53 deleted the afficher-message-cache-ancre branch July 9, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Front Concerne l'interface du site S-Évolution Ajoute de nouvelles fonctionnalités
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants