-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Image banner buttons alignment #793
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely fixes the centering issue 👍 , but I suspect we hadn't noticed this ourselves because when these buttons don't have links, they're actually positioned differently and don't create the alignment issue in this case (see here)
I think we should change the logic around adding the banner__buttons--multiple
class so that these buttons display/align the same whether they have links or not. It currently only gets added when both buttons have links. Let me know if you disagree or want to handle in another issue though.
@@ -91,7 +91,7 @@ | |||
<span>{{ block.settings.text | escape }}</span> | |||
</div> | |||
{%- when 'buttons' -%} | |||
<div class="banner__buttons{% if block.settings.button_label_1 != blank and block.settings.button_link_1 != blank and block.settings.button_label_2 != blank and block.settings.button_link_2 != blank %} banner__buttons--multiple{% endif %}" {{ block.shopify_attributes }}> | |||
<div class="banner__buttons{% if block.settings.button_label_1 != blank and block.settings.button_label_2 != blank %} banner__buttons--multiple{% endif %}" {{ block.shopify_attributes }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmeleta I've updated the logic to use the banner__buttons--multiple
class. As it makes sense to apply it whether there are links or not.
What do you think? Were you thinking of a different approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that's exactly what I had in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🎉
Why are these changes introduced?
Fixes #734
What approach did you take?
Added
margin-left: auto;
,margin-right: auto;
on the parent element of the buttons.Other considerations
Demo links
Checklist