Project

General

Profile

Development #33680

styler de la fonctionnalité +1 dans les formulaires

Added by Serghei Mihai 6 months ago. Updated 27 days ago.

Status:
Solution déployée
Priority:
Normal
Assignee:
Target version:
-
Start date:
05 Jun 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

0001-forms-decorate-nearby-field-33680.patch View (7.46 KB) Serghei Mihai, 18 Sep 2019 04:25 PM

signalement-lomme.png View (402 KB) Serghei Mihai, 18 Sep 2019 04:25 PM

signalement-nancy.png View (287 KB) Serghei Mihai, 18 Sep 2019 04:25 PM

signalement-montreuil.png View (1.27 MB) Serghei Mihai, 18 Sep 2019 04:25 PM

0001-forms-decorate-nearby-field-33680.patch View (7.67 KB) Serghei Mihai, 22 Sep 2019 09:42 PM

0001-forms-decorate-nearby-field-33680.patch View (8.13 KB) Serghei Mihai, 23 Sep 2019 10:25 AM

0001-forms-decorate-nearby-field-33680.patch View (8.28 KB) Serghei Mihai, 30 Sep 2019 10:27 AM

0001-forms-decorate-nearby-field-33680.patch View (8.31 KB) Serghei Mihai, 30 Sep 2019 02:59 PM

0001-forms-decorate-nearby-field-33680.patch View (8.41 KB) Serghei Mihai, 04 Oct 2019 05:29 PM

0001-forms-decorate-nearby-field-33680.patch View (8.38 KB) Serghei Mihai, 08 Oct 2019 04:30 PM

0001-forms-decorate-nearby-field-33680.patch View (8.5 KB) Serghei Mihai, 10 Oct 2019 04:21 PM

0001-forms-decorate-nearby-field-33680.patch View (8.55 KB) Serghei Mihai, 13 Nov 2019 11:51 AM

37390
37391
37392

Associated revisions

Revision 80bea41c (diff)
Added by Serghei Mihai about 1 month ago

forms: decorate "nearby" field (#33680)

History

#1 Updated by Serghei Mihai 3 months ago

37390
37391
37392

Reprise de la partie Javascript du thème Toodego.
Les libellé et le sommaire des demandes voisines seront à poser dans des variables de traitement au début du workflow.

Exemples de quelques rendus.

#2 Updated by Frédéric Péters 3 months ago

Appeler ça map--nearby-list.html, par rapport au nommage dans toodego nearby-list.html, ça ne va pas prendre le pas et casser Toodego ? Aussi le patch s'applique sur un fichier qui existerait déjà avant alors que non ?

Le côté tableau pas aligné est pas terrible. Plutôt qu'imposer des identifiants de champs (problem_title, problem_summary) pourquoi ne pas utiliser le digest, qui propose déjà un endroit pour définir un résumé succinct du contenu ? (ça empêche pas de taper ça dans un {% block whatever %} pour facilement pouvoir être adapté ailleurs.

Ailleurs, sur la généralisation du signalement (#33595), je note « (peut-être?) rendre optionnel l'affichage du "plus1" », perso ça m'irait bien ça impose moins de chose sur le workflow et ça nous évite de trop nous avancer dans ce bricolage.

Dans ce même autre ticket je note à propos du js "vraisemblablement le placer dans le gabarit" mais ce n'est quand même pas terrible à gérer, ça pourrait aller dans un fichier js/whatever et qu'il y ait juste <script src="..."> utilisé ? (oui il y a dedans un {% if user.is_authenticated %} à gérer différemment).

        &.marker-counter {
            background: $button-background;
            padding: 5px 10px;
            color: white;

plutôt utiliser $button-color, pour éviter le blanc sur blanc.

                background: lighten($primary-color, 10);

Le +1 apparait déjà comme un bouton via la classe pk-button, qui va assurer que $button-hover-background soit utilisé, c'est mieux que venir imposer ici une couleur alternative.

Sur les trucs flex, en attendant de peut-être avoir un truc pour gérer ça automatiquement, il faut ajouter les préfixes pour les vieux IE.

#3 Updated by Serghei Mihai 3 months ago

Je suis plutôt pour garder le JS inline pour maîtriser le détection d'un utilisateur connecté, ajout du bout d'ajax pour l'action +1 si elle est active.

#4 Updated by Frédéric Péters 3 months ago

maîtriser le détection d'un utilisateur connecté

Il y a une classe posée sur <body> qui peut t'assurer ça. (authenticated-user vs anonymous-user).

#5 Updated by Frédéric Péters 3 months ago

(d'ailleurs j'écrivais ça sans regarder mais c'est exactement ce qui est fait dans le code existant côté toodego)

#7 Updated by Frédéric Péters 3 months ago

Je posais la question :

Appeler ça map--nearby-list.html, par rapport au nommage dans toodego nearby-list.html, ça ne va pas prendre le pas et casser Toodego ?

En l'état, ça casse Toodego mais c'est plutôt à cause des CSS, ce serait chouette d'éviter (et la voie de sortie la plus simple est peut-être d'appeler ça différemment, genre nearby-items.html ?).

J'écrivais aussi :

Sur les trucs flex, en attendant de peut-être avoir un truc pour gérer ça automatiquement, il faut ajouter les préfixes pour les vieux IE.

Et c'est encore valable dans div#nearby-list. Aussi on met les propriétés préfixées en premier.

~~

À part ça je ne sais pas comment ça marche chez toi, côté firefox et chromium il ne prend pas le js sans faire ça :

-<script type="text/javascript" src="{% static "js/plus1.js" %}" />
+<script type="text/javascript" src="{% static "js/plus1.js" %}"></script>

(aussi, le js n'est pas seulement utilisé pour les +1, devrait tout le temps être chargé)

#8 Updated by Thomas Jund 3 months ago

-<script type="text/javascript" src="{% static "js/plus1.js" %}"></script>
+<script src="{% static "js/plus1.js" %}"></script>

l'attribut type n'est plus utile avec un doctype html5. Et oui la balise doit se fermer.

#9 Updated by Serghei Mihai 3 months ago

Etrange effectivement car sans la balise fermante mon firefox chargeait le JS.
J'ai rénommé le template en nearby-forms pour plus de clarté.

Préfixes IE pour les flexbox ajoutés.

#10 Updated by Frédéric Péters 3 months ago

Il y a moyen de limiter div.leaflet-div-icon aussi (parce que là, ça casse aussi toodego) ?

#11 Updated by Thomas Jund 3 months ago

JS l 2-3

  function user_is_authenticated() {
    - return $('body.authenticated-user').length;
    + return $('body').is('.authenticated-user');
  }

C'est pas plus lisible ? au moins on retourne un booleen et pas un number.

Ou en full JS

    document.body.classList.contain('.authenticated-user');

#12 Updated by Serghei Mihai 3 months ago

Avec limitation de la feuille de style aux markers du widget et la suggestion de ThomasJ pour la fonction user_is_authenticated.

#13 Updated by Thomas Jund 3 months ago

Autre petite proposition d'amélioration Js

  -var $map_widget = $('.qommon-map');
  -var map = $map_widget[0].leaflet_map;
  +var map = document.querySelector('.qommon-map').leaflet_map;

Tu charges un DomElement dans un objet jQuery ($map), pour ensuite chercher un objet de ce même domElement au sein de l'objet jQuery (map), Pour ensuite dans le code réinjecté cet élément dans un nouvel objet jQuery ($(map)).
Parfois Js natif est plus simple, non ?

#14 Updated by Thomas Jund 3 months ago

Autres suggestions au niveau HTML:

Un peu plus de sémantique ne fait jamais de mal.

Utiliser une ul > li pour les id="nearby-forms"

<ul id="nearvy-forms">
    <li …
    <li

le +1 ne devrait pas être une balise <a> puisqu'il ne mène nul part. La bonne pratique serait d'utiliser <button>

<button type="button" class="plus1 pk-button" …>+1</button>
<a class="plus1 pk-button" href="#" data-plus1-url="{{formdata.api_url}}hooks/plus1/">+1</a>

Et avec un attribut aria-label explicite. "+1" Ne l'est pas assez je pense.

La span comprenant la date devrait afficher la date au sein d'une balise <time>

<span class="problem-datetime">Signalé le <time datetime="{{ formdata.receipt_date|date:"c" }}" >{{ formdata.receipt_date }}</time></span>

Et enfin (promis après j'arrête)
Pour centrer un flex-item unique verticalement et horizontalement, il suffit de lui appliqué un margin:auto

  div.template-nearby-forms div.leaflet-div-icon {
-       padding: 3px;
    border-radius: 3px;
    display: -ms-flexbox;
    display: flex;
-    -ms-flex-pack: space-around;
-    -ms-justify-content: space-around;
-    justify-content: space-around;
    a {
-        line-height: 1em;
+        line-height: 1;
+               margin: auto;
    }
}

#15 Updated by Serghei Mihai 2 months ago

Thomas Jund a écrit :

Autres suggestions au niveau HTML:

Un peu plus de sémantique ne fait jamais de mal.

Utiliser une ul > li pour les id="nearby-forms"

Ok.

le +1 ne devrait pas être une balise <a> puisqu'il ne mène nul part. La bonne pratique serait d'utiliser <button>

Un button signifie une action, hors ici il s'agit d'un présentation de l'index de la demande. J'utilise plutôt un span.

[...]
Et avec un attribut aria-label explicite. "+1" Ne l'est pas assez je pense.

Je rajoute "Même signalement".

La span comprenant la date devrait afficher la date au sein d'une balise <time>

Ok.

Pour centrer un flex-item unique verticalement et horizontalement, il suffit de lui appliqué un margin:auto

Ok, mais je garde le padding pour que la bordure ne colle pas trop au texte.

#19 Updated by Thomas Jund 2 months ago

Débat sur la convention de nommage ouvert : https://dev.entrouvert.org/issues/36831

Sinon à corriger html l37

-    <button class="plus1" href="#" data-plus1-url="{{formdata.api_url}}hooks/plus1/" aria-label="Même signalement">+1</button>
+    <button role="button" class="plus1" data-plus1-url="{{formdata.api_url}}hooks/plus1/" aria-label="Même signalement">+1</button>

Ajouter au <button> un role=button. Oui c'est bizarre mais par defaut il a un "role=submit". Et enlever l'attribut href.

#20 Updated by Serghei Mihai 2 months ago

En attendant qu'on se mette d'accord j'ai explicité les noms des classes.
Et la correction du bouton.

#21 Updated by Thomas Jund 2 months ago

Astuce,
tu peux continuer à utiliser le nesting de sass sans générer de selecteur enfant.
Pour arriver au même résultat que tu as là, ajoute juste une class .nearby-forms à ton element #nearby-forms et une class nearby-forms-list à l'élément li

et tu peux écrire

.nearby-forms {
    margin: 0;
    padding: 0;

    &-list {…}

    &-digest, &-datetime, &-marker-counter {…}

    &-digest {…}
    &-datetime {…}
    &-marker-counter {…}
}

#22 Updated by Serghei Mihai 2 months ago

Oui, mais ça réduit beaucoup la lisibilité je trouve.

#23 Updated by Thomas Jund about 1 month ago

Niveau convention, on est parti sur un format block--element.
Tu pourrais donc partir sur des noms de class comme

<section
<div id="map-{{widget.name}}" class="qommon-map nearby-forms--map">
<ul class="nearby-forms">
    <li class="nearby-form">
        <span class="nearby-form--marker-counter" >
        <span class="nearby-form--digest">
        <span class="nearby-form--datetime">
        <button class="nearby-form--plus1">

#25 Updated by Thomas Jund about 1 month ago

Je me suis rendu compte dernièrement que IE11 était compatible avec la spec flexbox. Il n'est donc plus nécessaire de mettre les -ms-trucmuch qui sont nécessaires uniquement pour le support de IE10 (et qu'on ne support plus).

#28 Updated by Serghei Mihai about 1 month ago

D'autres remarques? J'aimerais bien envoyer en recette rapidement.

#29 Updated by Thomas Jund about 1 month ago

  • Status changed from Solution proposée to Solution validée

#30 Updated by Serghei Mihai about 1 month ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 80bea41c15c64f84a1aba0b0ac75a8043e6852b8 (origin/master, origin/HEAD)
Author: Serghei Mihai <smihai@entrouvert.com>
Date:   Wed Jun 5 10:31:14 2019 +0200

    forms: decorate "nearby" field (#33680)

#31 Updated by Frédéric Péters 27 days ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF