Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Location: Ajout d'un lien "en savoir plus" #537

Merged
merged 22 commits into from
Sep 12, 2024

Conversation

Clararigaud
Copy link
Contributor

@Clararigaud Clararigaud commented Aug 5, 2024

Type

  • Nouvelle fonctionnalité
  • Bug
  • Ajustement
  • Rangement

Description

Ajout du bouton "more" en layout grid.

Manque le design dans le figma. ( @arnaudlevy )

Problème à regler ( @arnaudlevy , @SebouChu , @pabois ) : le parametre de layout grid ou list n'est pas passé. ( example
osunyorg/admin#2112

Niveau d'incidence

  • Incidence faible 😌
  • Incidence moyenne 😲
  • Incidence forte 😱

Référence (ticket et/ou figma)

URL de test sur example.osuny.org

[branch]--example.osuny.netlify.app

URL de test du site (optionnel)

Screenshots

Layout grid
image
image

Layout list
image
image

Mobile
image

@Clararigaud Clararigaud changed the title Block locations adding button option Block Location: Ajout d'un lien "en savoir plus" Aug 6, 2024
@Clararigaud
Copy link
Contributor Author

TODO: integrer selon figma

@Clararigaud
Copy link
Contributor Author

ready @Olivia206 @arnaudlevy

Copy link
Member

@arnaudlevy arnaudlevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pas sûr de moi du tout mais je trouve le css compliqué et je vois pas bien pourquoi

.media
a
margin-top: 0
.location-content
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tout ça est bien compliqué, @Olivia206 tu peux simplifier tu crois ?

@Clararigaud
Copy link
Contributor Author

J'ai simplifié le css (@arnaudlevy ) et j'ai remis le lien sur le titre (@Olivia206 )

Full width:

image image

Sidebar:

image image

Mobile:

image

Copy link
Member

@arnaudlevy arnaudlevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je sais pas trop !

assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
layouts/partials/locations/location.html Show resolved Hide resolved
Copy link
Contributor

@Olivia206 Olivia206 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai fait une première passe sur le style, je te laisse me dire s'il y a des points à discuter ou pour lesquels je n'ai pas été assez claire @Clararigaud !

layouts/partials/locations/location.html Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Show resolved Hide resolved
@Clararigaud
Copy link
Contributor Author

Coucou j'ai tout repris à partir de vos commentaires. A priori ca devrait etre bon mais je veux bien une derniere mini verif @arnaudlevy et @Olivia206

@Clararigaud
Copy link
Contributor Author

( bon avec la bordure inspirée de posts.sass @Olivia206 :) )

@Olivia206
Copy link
Contributor

Le texte doit être sur la 5e colonne si je ne me trompe pas :

Capture d’écran 2024-09-05 à 17 44 53

Copy link
Contributor

@Olivia206 Olivia206 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quelques petits correctifs, attention à bien tester ton code 🙌

assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
assets/sass/_theme/sections/locations.sass Outdated Show resolved Hide resolved
@Clararigaud
Copy link
Contributor Author

Merci pour ta relecture @Olivia206 !!
J'ai appliqué tes suggestions et j'ai bien tout retesté, ca a l'air bon de mon coté.
Effectivement j'ai tendance à procéder selon cette logique > 1) Mobile -- 2) override de cas desktop / sidebar.
Mais en fin de compte le bi-directionnel permet d'alleger pas mal.

Le max-width sur l'image c'était parce que le width n'était pas pris en compte, mais maintenant c'est bon alors tout va bien :)

@Olivia206
Copy link
Contributor

Olivia206 commented Sep 12, 2024

Tu as raison de faire comme ça, mais l'idéal c'est de ni se répéter, ni écraser, il n'empêche qu'on peut tout à fait écraser si ça facilite la lecture du code et que ça évite de se répéter ! Je vais essayer de rassembler des exemples pour écrire un bout de doc là dessus

Petit souci ici (http://localhost:1313/fr/blocks/blocs-de-liste/campus-1/ | http://localhost:1313/fr/campus/) en pleine page :

Capture d’écran 2024-09-12 à 12 00 27

@Clararigaud
Copy link
Contributor Author

fixed @Olivia206
Screenshot 2024-09-12 at 16 41 14

@Olivia206
Copy link
Contributor

Olivia206 commented Sep 12, 2024

Tu peux p-e mettre un line-height: 1 sur le .more pour être plus proche du figma ? Ça réduit un peu plus l'espace entre le bas de l'image et le texte.

Dis-moi si c'est surfait haha

@@ -13,7 +13,9 @@
@include meta
@include icon(arrow-right-line, after)
margin-left: $spacing-2
line-height: 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je serais d'avis de mettre ça uniquement dans le layout liste en pleine largeur, tu en penses quoi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c'est fait !

@Olivia206
Copy link
Contributor

C'est bon j'ai fini de t'embêter 💪

@Clararigaud Clararigaud merged commit 7a561fb into main Sep 12, 2024
3 checks passed
@Clararigaud Clararigaud deleted the block-locations-adding-button-option branch September 12, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants