Bug #40717
carrousel: permettre le positionnement du texte en haut à droite du bloc
0%
Description
En plus du coin bas à gauche.
Fichiers
Révisions associées
clermont-metropole: update carrousel content position (#40717)
metz-metropole-2019: fix carrousel content position (#40717)
quimper: update carrousel content position (#40717)
entrouvert: update carrousel content position (#40717)
signal-publik: update carrousel content position (#40717)
haute-garonne-cd31: update carrousel content position (#40717)
nancy-en-direct: update carrousel content position (#40717)
lille: update carrousel content position (#40717)
arles-2020: update carrousel content position (#40717)
Historique
Mis à jour par Serghei Mihai il y a environ 4 ans
- Fichier 0001-carrousel-allow-text-position-on-top-right-corner-40.patch 0001-carrousel-allow-text-position-on-top-right-corner-40.patch ajouté
- Tracker changé de Support à Bug
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
Mis à jour par Frédéric Péters il y a environ 4 ans
Ça ne fait pas bizarre d'avoir le texte en haut à droite mais aligné à gauche ?
Je note ça surtout parce que l'imbrication m'y fait penser, mais elle me fait surtout dire que ça serait plus clair de ne pas imbriquer ainsi, d'en rester à un niveau,
@if $carrousel-text-position == "middle" {
...
} @else if $carrousel-text-position == "bottom-left" {
...
+} @else if $carrousel-text-position == "top-right" {
Aussi, mettre à jour le commentaire en haut de fichier qui mentionne les valeurs possibles, et la page de documentation (help/fr/misc-scss.page).
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
Je rejoins Fred sur ses remarques.
Je me demande aussi s'il ne serait pas préférable de passer à flexbox pour le positionning, ce qui permettrait à terme d'utiliser simplement des paddings sur `carrousel-item-content`pour éviter les superpositions avec les boutons de navigations (bullet & arrows).
@@ -37,7 +37,6 @@ div.carrousel-content {
pointer-events: none;
transition: opacity ease 0.5s;
div.carrousel-item {
- display: table;
position: relative;
background-position: center center;
box-sizing: border-box;
@@ -63,7 +62,11 @@ div.carrousel-content {
box-sizing: border-box;
}
position: relative;
+ display:flex;
+ height: 100%;
@if $carrousel-text-position == "middle" {
+ align-items: center;
+ justify-content: center;
> a {
padding: 1rem;
margin-left: 6rem;
@@ -73,16 +76,17 @@ div.carrousel-content {
margin-right: 2rem;
}
}
- display: table-cell;
- vertical-align: middle;
p {
margin-left: auto;
margin-right: auto;
}
} @else if $carrousel-text-position == "bottom-left" {
+ align-items: flex-end;
- display: table;
position: relative;
background-position: center center;
box-sizing: border-box;
@@ -63,7 +62,11 @@ div.carrousel-content {
box-sizing: border-box;
}
position: relative;
+ display:flex;
+ height: 100%;
@if $carrousel-text-position == "middle" {
+ align-items: center;
+ justify-content: center;
> a {
padding: 1rem;
margin-left: 6rem;
@@ -73,16 +76,17 @@ div.carrousel-content {
margin-right: 2rem;
}
}
- display: table-cell;
- vertical-align: middle;
p {
margin-left: auto;
margin-right: auto;
}
} @else if $carrousel-text-position == "bottom-left" {
+ align-items: flex-end;
text-align: left;
- position: absolute;
bottom: 1rem;
+ } @else if $carrousel-text-position == "top-right" {
+ justify-content: flex-end;
+ text-align: right;
}
z-index: 100;
Mis à jour par Serghei Mihai il y a environ 4 ans
- Assigné à mis à Serghei Mihai
Ok, je vais d'abord refactoriser l'existant sur la base de tes arguments.
Mis à jour par Serghei Mihai il y a environ 4 ans
- Fichier 0002-scss-allow-carrousel-content-position-on-top-right-c.patch 0002-scss-allow-carrousel-content-position-on-top-right-c.patch ajouté
- Fichier 0001-scss-refactor-carrousel-content-positionning-40717.patch 0001-scss-refactor-carrousel-content-positionning-40717.patch ajouté
Voilà. Fait le tour des intégrations existantes utilisant bottom-left, ça m'a l'air ok.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
- Tu as des conditions à la fois dans `.carrousel-content` et dans `.carrousel-item-content`. Je pense qu'il est possible de rassembler les conditions dans l'un ou l'autre uniquement pour simplifier le code sass.
- un display:table qui traine encore
- ajouter le `text-align: center` à la condition `middle`
@@ -36,7 +36,6 @@ div.carrousel-content {
pointer-events: none;
transition: opacity ease 0.5s;
div.carrousel-item {
- display: table;
position: relative;
background-position: center center;
box-sizing: border-box;
@@ -59,12 +58,15 @@ div.carrousel-content {
@if $carrousel-text-position == "middle" {
align-items: center;
justify-content: center;
+ text-align: center;
} @else if $carrousel-text-position == "bottom-left" {
align-items: flex-end;
justify-content: flex-start;
+ text-align: left;
} @else if $carrousel-text-position == "top-right" {
align-items: flex-start;
justify-content: flex-end;
+ text-align: right;
}
div.carrousel-item-content {
@if $carrousel-navigation == "visible" {
@@ -89,10 +91,6 @@ div.carrousel-content {
margin-left: auto;
margin-right: auto;
}
- } @else if $carrousel-text-position == "bottom-left" {
- text-align: left;
- } @else if $carrousel-text-position == "top-right" {
- text-align: right;
}
z-index: 100;
line-height: 110%;
@@ -102,7 +100,6 @@ div.carrousel-content {
}
color: white;
font-size: 120%;
- text-align: center;
}
}
Mis à jour par Serghei Mihai il y a environ 4 ans
- Fichier 0002-scss-allow-carrousel-content-position-on-top-right-c.patch 0002-scss-allow-carrousel-content-position-on-top-right-c.patch ajouté
- Fichier 0001-scss-refactor-carrousel-content-positionning-40717.patch 0001-scss-refactor-carrousel-content-positionning-40717.patch ajouté
Effectivement on peut taper l'alignement dans le parent.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
- Statut changé de Solution proposée à Solution validée
Pour les besoins de ce ticket, pour moi c'est bon
Mis à jour par Frédéric Péters il y a environ 4 ans
- Statut changé de Solution validée à En cours
Nope.
Mis à jour par Frédéric Péters il y a environ 4 ans
L'exigence IE10 pour GNM n'a pas encore été levée.
Mis à jour par Frédéric Péters il y a environ 4 ans
Aussi, mon commentaire initial : mettre à jour le commentaire en haut de fichier qui mentionne les valeurs possibles, et la page de documentation (help/fr/misc-scss.page) (cette partie ok).
Mis à jour par Frédéric Péters il y a environ 4 ans
Aussi, ça "casse" l'intégration Metz Métropole, où le texte est désormais centré.
Mis à jour par Serghei Mihai il y a environ 4 ans
- Fichier Capture_d’écran__1_.png Capture_d’écran__1_.png ajouté
En emulant avec Edge le IE 10 j'ai l'impression que le rendu n'est pas cassé.
Pour le thème de Metz j'ai modifié le custom vu que la navigation du carrousel a été mise à droite.
Branche à jour.
Mis à jour par Frédéric Péters il y a environ 4 ans
Et les autres intégrations qui modifient du comportement du carrousel, tout va bien ? (arles-2020 clermont-metropole entrouvert haute-garonne-cd31 lille meyzieu-2018 nancy-en-direct quimper)
En emulant avec Edge le IE 10 j'ai l'impression que le rendu n'est pas cassé.
Un vrai IE10 ne gère pas les flexbox sans préfixe.
Mis à jour par Serghei Mihai il y a environ 4 ans
Frédéric Péters a écrit :
L'exigence IE10 pour GNM n'a pas encore été levée.
Oui. IE10 n'est plus utilisé. Cédric LAMBERT
On garde toujours l'exigence?
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
ah ben non, j'attendais que ça, moi, pour dégager tous les préfixes et mixins flexbox.
Mis à jour par Frédéric Péters il y a environ 4 ans
Clairement, et je poussais pour avoir une réponse pour pouvoir faire ça.
Mis à jour par Serghei Mihai il y a environ 4 ans
- Fichier lille.png lille.png ajouté
- Fichier quimper.png quimper.png ajouté
- Fichier cd31.png cd31.png ajouté
- Fichier entrouvert.png entrouvert.png ajouté
- Fichier clermont.png clermont.png ajouté
- Fichier arles.png arles.png ajouté
Frédéric Péters a écrit :
Et les autres intégrations qui modifient du comportement du carrousel, tout va bien ? (arles-2020 clermont-metropole entrouvert haute-garonne-cd31 lille meyzieu-2018 nancy-en-direct quimper)
Arles et Nancy adaptées en utilisant l'option "top-right" pour aligner le contenu.
Meyzieu n'utilise pas le menu carrousel.
Toutes les autres integrations me semblent ok, sauf quimer ou j'ai pas trouvé de carrousel en recette ou prod pour comparer.
Mis à jour par Frédéric Péters il y a environ 4 ans
C'est peut-être lié au nombre de lignes mais première capture (lille) les ronds de navigations sont collés de trop près, ce n'est pas ainsi en prod.
Quimper de fait ça a été retiré :/ https://www.quimperplus.bzh/index_old/
Mis à jour par Serghei Mihai il y a environ 4 ans
- Fichier lille.mobile.1.png lille.mobile.1.png ajouté
- Fichier lille.1.png lille.1.png ajouté
Effectivement à Lille il manquait la position absolue pour remettre tout en état.
Le rendu à Quimper est le même.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
- Fichier 0001-scss-carrousel-add-news-text-positions-combinaisons.patch 0001-scss-carrousel-add-news-text-positions-combinaisons.patch ajouté
Le code de Serghei me va bien en l'état, mais je propose d'aller plus loin.
Serghei a fait le plus gros du boulot en basculant le layout en flexbox et vérifiant la compatibilité avec l'existant.
Il serait vraiment dommage de recommencer ce travail dans quelques mois pour ajouter une position "top-left".
Pourquoi ne pas tout de suite coder les 9 positions possibles par flexbox :
vertical options: top | middle | bottom
horizontal options: left | middle | right.
Je propose donc d'utiliser les list sass pour définir 2 valeurs séparés par un espace :
$carrousel-text-position: top right;
Le code sass suivant ne surcharge pas du tout le CSS existant et permet les 9 positions
// Texte Position options
@if ($carrousel-text-position == middle) {
$carrousel-text-position: middle middle;
}
// vertical position
$carrousel-text-vertical-position: nth($carrousel-text-position, 1);
// horizontal position
$carrousel-text-horizontal-position: nth($carrousel-text-position, 2);
.carrousel-item {
@if $carrousel-text-vertical-position == top {
align-items: flex-start;
} @else if $carrousel-text-vertical-position == middle {
align-items: center;
} @else if $carrousel-text-vertical-position == bottom {
align-items: flex-end;
}
@if $carrousel-text-horizontal-position == left {
justify-content: flex-start;
text-align: left;
} @else if $carrousel-text-horizontal-position == middle {
justify-content: center;
text-align: center;
} @else if $carrousel-text-horizontal-position == right {
justify-content: flex-end;
text-align: right;
}
}
Voici le patch
Aside note: Il semblerait que libsass, utilisé par sassc, soit en retard sur Dartsass et ne supporte pas encore les modules sass. D'où l'impossibilité pour l'instant d'utiliser les `build-in modules` comme sass:list listés dans la doc et son support n'est pas prévu avant la version 4 (https://github.com/sass/libsass/issues/2807). La documentation en ligne ne fait donc plus référence pour sassc.
Voilà pourquoi j'ai dût utiliser `nth()` à la place de `list:nth()`
Mis à jour par Serghei Mihai il y a environ 4 ans
Tu as raison.
Je fais le tour des integrations existantes pour vérifier le rendu.
Mis à jour par Serghei Mihai il y a environ 4 ans
Le rendu est correct partout, comme dans les captures que j'ai jointes précedemment.
J'ai inclut ton commit et reorganisé le reste.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
Super,
Il reste juste la propriété `display: flex` qui traine encore ligne 61 et devrait être remontée en 43 et pour moi c'est bon.
Mis à jour par Thomas Jund (congés, retour le 29/04) il y a environ 4 ans
- Statut changé de En cours à Solution validée
Mis à jour par Serghei Mihai il y a environ 4 ans
Je vais pousser après la mise à jour de jeudi pour éviter les surprises.
Mis à jour par Serghei Mihai il y a environ 4 ans
- Statut changé de Solution validée à Résolu (à déployer)
commit 1208c915b4c7b85eaaa7c2fd6c3e6a9e73e4cf38 (origin/master, origin/HEAD)
Author: Serghei Mihai <smihai@entrouvert.com>
Date: Fri Mar 20 13:11:38 2020 +0100
arles-2020: update carrousel content position (#40717)
commit 9664238136d836f4fe8304dc7e361b575ec51353
Author: Serghei Mihai <smihai@entrouvert.com>
Date: Fri Mar 20 14:36:16 2020 +0100
lille: update carrousel content position (#40717)
commit fd5cf798b7340c2560df6360f21bbaaaff091e87
Author: Serghei Mihai <smihai@entrouvert.com>
Date: Fri Mar 20 12:32:30 2020 +0100
nancy-en-direct: update carrousel content position (#40717)
commit 282c105f9bcabc7ed715e2abe1afd33924ebe364
Author: Thomas JUND <tjund@entrouvert.com>
Date: Mon Mar 23 12:04:33 2020 +0100
haute-garonne-cd31: update carrousel content position (#40717)
commit bbe15dc264f585d1401d3d0f9e9dc6d51333c9b7
Author: Thomas JUND <tjund@entrouvert.com>
Date: Mon Mar 23 12:04:14 2020 +0100
signal-publik: update carrousel content position (#40717)
commit d065afba1014083de0e81d7fee8b2728a7d31339
Author: Thomas JUND <tjund@entrouvert.com>
Date: Mon Mar 23 10:31:11 2020 +0100
entrouvert: update carrousel content position (#40717)
commit 2ecd1b30fea5aaa34b554e929f6ee939e5f5b982
Author: Thomas JUND <tjund@entrouvert.com>
Date: Mon Mar 23 10:26:33 2020 +0100
quimper: update carrousel content position (#40717)
commit 23ebcab9d2a93aeead024bd67f33682296ab7029
Author: Serghei Mihai <smihai@entrouvert.com>
Date: Wed Mar 18 09:27:37 2020 +0100
metz-metropole-2019: fix carrousel content position (#40717)
commit 825e8c7682df39c46dad21c4771359a5b9b9b888
Author: Thomas JUND <tjund@entrouvert.com>
Date: Mon Mar 23 10:26:01 2020 +0100
clermont-metropole: update carrousel content position (#40717)
commit 3bb1b3e2f05da28c942cb24be5509a2b8060ad4f
Author: Thomas JUND <tjund@entrouvert.com>
Date: Mon Mar 16 09:53:13 2020 +0100
scss: refactor carrousel content positionning (#40717)
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
scss: refactor carrousel content positionning (#40717)