Development #47550
publik, menu via le portail agent
0%
Fichiers
Demandes liées
Révisions associées
js: reduce number of jquery calls (#47550)
Historique
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Fichier 0001-portal-agent-make-it-possible-to-create-publik-menu-.patch 0001-portal-agent-make-it-possible-to-create-publik-menu-.patch ajouté
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Lié à Development #45741: publik, menu via le portail agent ajouté
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Lié à Project management #41402: construire le menu latéral du portail agent à partir de pages combo ajouté
Mis à jour par Thomas Noël il y a plus de 3 ans
Au niveau de :
if (window.location.href.indexOf(element.url) == 0) {
$(li).addClass('active');
}
je pense pas que ça active la page la plus précise.
Si je suis sur https://agent/page1/ et que j'ai un item vers https://agent/page1/page2/ puis un suivant vers https://agent/page1/ les deux vont être activés...
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a plus de 3 ans
Je trouve qu'il y a trop d'injections DOM.
Une injection DOM est faite pour injecter une ul vide puis une injection pour chaque li.
Je pense qu'il faudrait décaler `$sidepage_menu.appendTo('#sidepage');` en fin de fonction : créer l'ul, la remplir avec les items et à la fin l'injecter dans le DOM.
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Fichier 0001-portal-agent-make-it-possible-to-create-publik-menu-.patch 0001-portal-agent-make-it-possible-to-create-publik-menu-.patch ajouté
Je pense qu'il faudrait décaler `$sidepage_menu.appendTo('#sidepage');` en fin de fonction : créer l'ul, la remplir avec les items et à la fin l'injecter dans le DOM.
Voilà.
Si je suis sur https://agent/page1/ et que j'ai un item vers https://agent/page1/page2/ puis un suivant vers https://agent/page1/ les deux vont être activés...
Ça me semble compliquer quand même pas mal l'affaire d'avoir à gérer ce cas hypothétique, ça m'irait bien de l'ignorer.
Mis à jour par Thomas Noël il y a plus de 3 ans
Frédéric Péters a écrit :
Si je suis sur https://agent/page1/ et que j'ai un item vers https://agent/page1/page2/ puis un suivant vers https://agent/page1/ les deux vont être activés...
Ça me semble compliquer quand même pas mal l'affaire d'avoir à gérer ce cas hypothétique, ça m'irait bien de l'ignorer.
On va quand même avoir le cas avec https://portail-agent/ et https://portail-agent/manage/ non ?
Mis à jour par Frédéric Péters il y a plus de 3 ans
En l'espèce non parce que l'entrée portail agent est https://.../index, du coup ça matche pas. (oui on peut dire que ça révèle un bug différent, vu que les pages du portail agent ne feront pas que celui-ci soit marqué avec "active", mais pareil je préfère là m'en accommoder) (qu'avoir à écrire du js que je trouve/rai illisible).
Mis à jour par Thomas Noël il y a plus de 3 ans
Frédéric Péters a écrit :
En l'espèce non parce que l'entrée portail agent est https://.../index,
Arf. Effectivement. Ok pour moi donc.
Je laisse la relecture finale à l'autre Thomas pour la partie injection DOM.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a plus de 3 ans
en créant $('<li>') avec jQuery, `li` est déjà un object jQuery, inutile de le réinjecter.
- var li = $('<li><a href="#">' + element.label + '</a></li>').appendTo($sidepage_menu);
+ var $li = $('<li><a href="#">' + element.label + '</a></li>').appendTo($sidepage_menu);
- $(li).find('a').attr('href', element.url);
+ $li.find('a').attr('href', element.url);
Je vais donner l'impression de pinailler parce que ça doit pas changer grand chose, mais on a 4x `$(li).find('a')`. Un petit `const a = $(li).find('a')` devrait alléger le process.
(Cette relecture me fait penser qu'il serait peut-être temps d'essayer d'introduire quelques guidelines de codage JS : utilisation de `const` et `let` à la place de var, l'utilisation de querySelector à la place de sizzle, l'utilisation de `el.classList` à la place de `$.addclass` serait un bon début)
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Fichier 0001-js-reduce-number-of-jquery-calls-47550.patch 0001-js-reduce-number-of-jquery-calls-47550.patch ajouté
Je vais donner l'impression de pinailler parce que ça doit pas changer grand chose, mais on a 4x `$(li).find('a')`. Un petit `const a = $(li).find('a')` devrait alléger le process.
J'ai tapé ces modifications dans un commit séparé, pour convrir tout le fichier.
~~
... quelques guidelines de codage JS ...
Ce serait très bien mais attention quand même au rejet et à ce que ça n'amène pas tout le js sur ta pomme. Je pense par exemple important d'accompagner ça d'explications parce que genre sizzle aucune idée de ce que ça veut dire.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a plus de 3 ans
- Statut changé de Solution proposée à Solution validée
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Statut changé de Solution validée à Résolu (à déployer)
commit 9b547983150d06329d0288c14a971359eed2df54
Author: Frédéric Péters <fpeters@entrouvert.com>
Date: Mon Oct 19 10:10:51 2020 +0200
js: reduce number of jquery calls (#47550)
commit 5b5c2ae7e77454f47ad240d7c349bd8be2193b4e
Author: Frédéric Péters <fpeters@entrouvert.com>
Date: Mon Jul 27 21:42:48 2020 +0200
portal agent: make it possible to create publik menu from pages (#47550)
Mis à jour par Frédéric Péters il y a plus de 3 ans
- Statut changé de Résolu (à déployer) à Solution déployée
portal agent: make it possible to create publik menu from pages (#47550)