Projet

Général

Profil

Bug #38419

supprimer $top-logo-width

Ajouté par Thomas Jund il y a plus de 4 ans. Mis à jour il y a environ 4 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Version cible:
-
Début:
11 décembre 2019
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

La variable $top-logo-width est obsolète.


Fichiers

Révisions associées

Révision 87755b77 (diff)
Ajouté par Thomas Jund il y a environ 4 ans

scss: remove obsolete $top-logo-width sass var (#38419)

Historique

#1

Mis à jour par Thomas Jund il y a environ 4 ans

  • Statut changé de Nouveau à En cours
  • Assigné à mis à Thomas Jund

`$logo-top-width ne sert à rien. Je propose sa suppression.

Seul les thèmes Amiens metro, Hautes-alpes-2018 et Montreuil applique une valeur à `$logo-top-width`

  • Hautes-Alpes: de façon inutile car le header publik n'est pas utilisé (à patcher)
  • Montreuil: pour ajuster le position du logo (à patcher)
  • Amiens metro: le thème doit conserver la variable, mais la suppression de la variable du core n’entraîne aucune régression. La variable devient spécifique à ce thème.
#2

Mis à jour par Thomas Jund il y a environ 4 ans

#3

Mis à jour par Frédéric Péters il y a environ 4 ans

Ok je craignais qu'il ait une valeur différente de 0 et que ça amène du grabuge derrière parce que des thèmes auraient fait margin-left: -30px pour compenser, genre.

Il y a un peu ça quand même, avec ce moment :

static/includes/_layout.scss:                   padding-left: $top-logo-width+70px;

mais on y mettra 70px ça sonnera arbitraire, il faudra sans doute à un moment dégager tout ce bout :

        @media screen and (max-width: $mobile-limit) {
                & h1 {
                        padding-left: $top-logo-width+70px;
                        background-position: 70px;
                }
        }

et il y aura à ce moment inspection plus approfondie à faire.

#4

Mis à jour par Thomas Jund il y a environ 4 ans

Moi je pense qu'il faudra refléchir à la façon dont on pourrait paramétrer le layout du header. Avec layout du logo et du titre.

        @media screen and (max-width: $mobile-limit) {
                & h1 {
                        padding-left: $top-logo-width+70px;
                        background-position: 70px;
                }
        }

Les 70px sont là pour laisser la place au burger qui remonte à côté du titre.
valeur approximative qui devrait plutôt utiliser $nav-menu-side. Mais $nav-menu-side n'est pas encore disponible, Pour cela il faudrait charger _nav.scss avant _layout.scss ou declarer $nav-menu-side au sein de _layout.scss.

#5

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

J'ai l'impression que Fred indique qu'entre autre, une marge gauche négative peut aussi influencer rendu de padding-left: $top-logo-width+70px; (https://www.smashingmagazine.com/2009/07/the-definitive-guide-to-using-negative-margins/).
Cette marge gauche sur le h1 est souvent modifiée, mais je ne relève pas de valeurs négatives. Donc pour moi ça semblerait bon, mais encore une fois je me sent trop léger en CSS ici (et je n'ai pas fait le tour des thèmes pour tester).

$ for F in $(find . -type f -name '*.scss'); do \
    L=$(cat -n $F | sed -ne '/h1 /, /}/ p' \
      | grep margin | grep -v top | grep -v bottom | grep -v 'margin: 0;'\
    ); \
    if [ $(echo $L|wc -w) -gt 0 ]; then echo "** $F **"; echo $L; fi; \
  done
** ./static/gorges/_custom.scss **
22 margin: 0 auto;
** ./static/saone-et-loire-cd71-2019/_custom.scss **
195 margin: 0 0.5rem; 206 margin: 0 0.5rem;
** ./static/seine-et-marne-apa/_custom.scss **
55 margin-left: 250px; 101 margin-left: 250px;
** ./static/toulouse-metropole/_custom.scss **
811 margin-right: 160px;
** ./static/saone-et-loire-cd71/_custom.scss **
38 margin-right: 10px;
** ./static/toulouse/_custom.scss **
665 margin: 0 50px; // space for "actus" button 788 margin-right: 160px; 790 margin-right: 0;
** ./static/groupe-up/_custom.scss **
37 margin-left: 305px;
** ./static/montpellier/_custom.scss **
82 margin-left: 20px;
** ./static/rochefort/_custom.scss **
74 margin-left: 22px;
** ./static/fontenay-sous-bois-2018/_custom.scss **
217 margin: 0 0.5rem; 228 margin: 0 0.5rem;
** ./static/pre-saint-gervais/_custom.scss **
104 margin: 0 auto;
** ./static/amiens-metropole/_custom.scss **
55 margin: $a-la-une-text-margin;
** ./static/calvados-cd14/_custom.scss **
49 margin-left: 1rem;
** ./static/quimper/_custom.scss **
43 margin-right: 10px;
** ./static/toodego/_custom.scss **
1059 margin: 1rem 0.5rem;

#6

Mis à jour par Thomas Jund il y a environ 4 ans

L'utilisation d'un margin-right negatif sur #header h1 n'est pas techniquement impossible, même si cela ferait passer le logo sous le burger menu en mobile.

Si le thème n'a pas alloué de valeur custom à $top-logo-width, il n'y a aucune raison que sa suppression impacte son thème. Marges négatives ou pas.

#7

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

ça me semble bon, mais à mois de rebaser, ce patch me semble bloqué par #37223 (je n'ai pas testé du coup)

#8

Mis à jour par Thomas Jund il y a environ 4 ans

J'ai utilisé $max-mobile-viewport pour patcher montreuil (je ne voulais pas oublier ensuite de le changer après validation de #37223).

#9

Mis à jour par Thomas Jund il y a environ 4 ans

#37223 étant validé et mergé, ce patch peut être relu.

#10

Mis à jour par Nicolas Roche (absent jusqu'au 3 avril) il y a environ 4 ans

  • Statut changé de Solution proposée à Solution validée

(par contre tu vas certainement devoir rebaser ton patch pour pouvoir l'appliquer)

#11

Mis à jour par Thomas Jund il y a environ 4 ans

  • Statut changé de Solution validée à Résolu (à déployer)
commit 87755b7788d717a3e7ae5f7ff130d919ba29f5c4 (HEAD -> master, origin/master, origin/HEAD)
Author: Thomas JUND <tjund@entrouvert.com>
Date:   Wed Jan 29 14:42:49 2020 +0100

    scss: remove obsolete $top-logo-width sass var (#38419)
#12

Mis à jour par Frédéric Péters il y a environ 4 ans

On peut éviter de taper ça la semaine de mise en prod.

#13

Mis à jour par Frédéric Péters il y a environ 4 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF