#120 Amelioration de la fonction lien_ou_expose fix #4552

Merged
cerdic merged 2 commits from issue_4552 into master 6 days ago
cerdic commented 1 week ago
Owner
There is no content yet.
cerdic added 1 commit 1 week ago
tcharlss commented 1 week ago
Poster
Owner

C'est un cas où la préservation de l'existant oblige à faire des choses assez compliquées quand même, je trouve.

Le rôle et l'utilisation du paramètre $on deviennent un peu confus, servant à la fois à indiquer si l'élément est exposé, et à la fois à indiquer une classe et une balise.
Suivi d'un paramètre pour donner une classe...

L'autre proposition c'était simplement de changer légèrement le sens du param $class pour dire que ça s'appliquait tout le temps, exposé ou pas. À partir de là, peu importe les balises, on a tout ce qu'il faut pour cibler en CSS l'élément, dans ses 2 états.
Au prix d'une règle CSS à changer chez les gens donc.

C'est un cas où la préservation de l'existant oblige à faire des choses assez compliquées quand même, je trouve. Le rôle et l'utilisation du paramètre $on deviennent un peu confus, servant à la fois à indiquer si l'élément est exposé, et à la fois à indiquer une classe *et* une balise. Suivi d'un paramètre pour donner une classe... L'autre proposition c'était simplement de changer légèrement le sens du param $class pour dire que ça s'appliquait tout le temps, exposé ou pas. À partir de là, peu importe les balises, on a tout ce qu'il faut pour cibler en CSS l'élément, dans ses 2 états. Au prix d'une règle CSS à changer chez les gens donc.
Poster
Owner

Il faut changer le PHPDoc d'ailleurs, pour expliquer ce qu'on peut mettre dans $on.

Et faut peut-être renommé $class plutôt en $class_lien par ex, pour bien signifier tout le long (et en premier lieu dans la doc automatique) que c'est la classe du lien quand c'est PAS exposé.

Il faut changer le PHPDoc d'ailleurs, pour expliquer ce qu'on peut mettre dans $on. Et faut peut-être renommé $class plutôt en $class_lien par ex, pour bien signifier tout le long (et en premier lieu dans la doc automatique) que c'est la classe du lien quand c'est PAS exposé.
cerdic commented 1 week ago
Poster
Owner

"Au prix d'une règle CSS à changer chez les gens donc." donc en pétant tous les sites, directement, gratuitement...

C'est pas "une règle CSS", c'est potentiellement N, pour tous les endroits où ce filtre est utilisé dans les squelettes, supposant donc de se replonger dans toutes les feuilles de style et de debug tes sites un par un.

C'est définitivement pas notre façon de faire dans SPIP, sauf quand on y est acculé.

Les gens doivent pouvoir mettre à jour en confiance, sinon une fois qu'ils ont eu une mauvaise expérience ils ne mettent pas à jour.

Par ailleurs ta proposition fait que du code écrit pour la nouvelle version de la fonction ne marche plus du tout sur une ancienne version, on a donc pas de dégradation gracieuse.

Note bien que tu peux continuer à utiliser la fonction exactement comme avant, sans complexifier le on.

J'avais permis de spécifier la balise dans le filtre pour l'état on car certains framework (BS en l'occurence dans mon cas), conservent parfois un <a> même dans l'état exposé, mais on peut simplifier et permettre de donner juste une ou des classes pour l'état exposé, ce qui permettrait de faire:

[(#URL_RUBRIQUE|lien_ou_expose{#TITRE, #ENV{test}|=={en_cours}|?{monlien},'monlien'})]

Mais note bien que si ton besoin c'est d'avoir tout le temps la class, le plus simple est donc de traiter https://core.spip.net/issues/3460
et de faire ensuite

[(#URL_RUBRIQUE|lien_ou_expose{#TITRE, #ENV{test}|=={en_cours}}|ajouter_class{monlien})]
"Au prix d'une règle CSS à changer chez les gens donc." donc en pétant tous les sites, directement, gratuitement... C'est pas "une règle CSS", c'est potentiellement N, pour tous les endroits où ce filtre est utilisé dans les squelettes, supposant donc de se replonger dans toutes les feuilles de style et de debug tes sites un par un. C'est définitivement pas notre façon de faire dans SPIP, sauf quand on y est acculé. Les gens doivent pouvoir mettre à jour en confiance, sinon une fois qu'ils ont eu une mauvaise expérience ils ne mettent pas à jour. Par ailleurs ta proposition fait que du code écrit pour la nouvelle version de la fonction ne marche plus du tout sur une ancienne version, on a donc pas de dégradation gracieuse. Note bien que tu peux continuer à utiliser la fonction exactement comme avant, sans complexifier le on. J'avais permis de spécifier la balise dans le filtre pour l'état on car certains framework (BS en l'occurence dans mon cas), conservent parfois un `<a>` même dans l'état exposé, mais on peut simplifier et permettre de donner juste une ou des classes pour l'état exposé, ce qui permettrait de faire: ``` [(#URL_RUBRIQUE|lien_ou_expose{#TITRE, #ENV{test}|=={en_cours}|?{monlien},'monlien'})] ``` Mais note bien que si ton besoin c'est d'avoir tout le temps la class, le plus simple est donc de traiter https://core.spip.net/issues/3460 et de faire ensuite ``` [(#URL_RUBRIQUE|lien_ou_expose{#TITRE, #ENV{test}|=={en_cours}}|ajouter_class{monlien})] ```
cerdic commented 1 week ago
Poster
Owner

Il faut changer le PHPDoc d'ailleurs, pour expliquer ce qu'on peut mettre dans $on.

Le PHPDoc est mis à jour dans la proposition hein :)

> Il faut changer le PHPDoc d'ailleurs, pour expliquer ce qu'on peut mettre dans $on. > Le PHPDoc est mis à jour dans la proposition hein :)
Poster
Owner

Le PHPDoc est mis à jour dans la proposition hein :)

eh non, pas la doc du @param $on qui est celui qui change justement :)

> Le PHPDoc est mis à jour dans la proposition hein :) eh non, pas la doc du @param $on qui est celui qui change justement :)
tcharlss commented 1 week ago
Poster
Owner

Bon je fais une parenthèse pour dire que tes réponses sont très cassantes pour rien Cedric, pleines de sous-entendus mesquins. Je sais pas si c'est fait exprès, mais ça donne vraiment pas envie de continuer la discussion. Dont acte.

Bon je fais une parenthèse pour dire que tes réponses sont très cassantes pour rien Cedric, pleines de sous-entendus mesquins. Je sais pas si c'est fait exprès, mais ça donne vraiment pas envie de continuer la discussion. Dont acte.
cerdic added 1 commit 1 week ago
cerdic commented 1 week ago
Poster
Owner

Désolé j'essaye d'aller à l'essentiel et de pas me perdre dans les discussions en passant d'un ticket à l'autre...

J'ai traité #3460 via #127 et du coup il me semble que ça réponds simplement à ton besoin d'avoir une classe qui soit toujours là, que le lien soit exposé ou non, indépendamment de cette PR

(concernant cette PR, on peut ou la fermer et considérer que le core reste tel quel ou l'intégrer quand même, les 2 me vont)

Désolé j'essaye d'aller à l'essentiel et de pas me perdre dans les discussions en passant d'un ticket à l'autre... J'ai traité #3460 via https://git.spip.net/spip/spip/pulls/127 et du coup il me semble que ça réponds simplement à ton besoin d'avoir une classe qui soit toujours là, que le lien soit exposé ou non, indépendamment de cette PR (concernant cette PR, on peut ou la fermer et considérer que le core reste tel quel ou l'intégrer quand même, les 2 me vont)
Poster
Owner

moi je trouve ça bien de l'intégrer, ça permet plein de choses quand même, pour générer le bon code par rapport aux frameworks utilisés, y compris avec la constante qui permet pour le site entier d'un coup

moi je trouve ça bien de l'intégrer, ça permet plein de choses quand même, pour générer le bon code par rapport aux frameworks utilisés, y compris avec la constante qui permet pour le site entier d'un coup
b_b commented 1 week ago
Poster
Owner

ça ne pète rien + ça améliore = +1 :)

et des zoubis pour @tcharlss & @cerdic : tranquillou les loulous :)

ça ne pète rien + ça améliore = +1 :) et des zoubis pour @tcharlss & @cerdic : tranquillou les loulous :)
cerdic merged commit 1275102077 into master 6 days ago
cerdic deleted branch issue_4552 6 days ago
The pull request has been merged as 1275102077.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.