-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Adjust conditional rendering of view all link in featured collection #251
Conversation
ad160b7
to
3f3c30f
Compare
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.
Tried to think of a way to do all this without the assign, but it doesn't feel possible or at least isn't intuitive, so I'm ok with it. 👍
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 🎉 Added a small comment about a class
assign products_to_display = section.settings.products_to_show | ||
assign more_in_collection = true | ||
endif | ||
-%} | ||
<div class="collection page-width{% if section.settings.swipe_on_mobile == true and section.settings.collection.all_products_count > 2 and section.settings.products_to_show > 2 %} page-width-desktop{% endif %}"> | ||
<div class="{% if section.settings.show_view_all and section.settings.swipe_on_mobile %}title-wrapper-with-link{% endif %}{% if section.settings.title == blank %} title-wrapper-with-link--no-heading{% endif %}{% if section.settings.collection.all_products_count > 2 and section.settings.swipe_on_mobile and section.settings.products_to_show > 2 %} title-wrapper--self-padded-tablet-down{% endif %}"> |
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.
Should we also be adding and more_in_collection
to line 22 so we dont add the class title-wrapper-with-link
when the button doesnt exist?
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.
That affects the padding a bit, so I think we should keep it as is, though I agree that it's a bit weird. We really need to revisit the whole title css as a whole. I think we had an issue for that.
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.
Didn't find the issue so I created one. I really don't like the title component and all its weird modifiers, etc... especially when we've got all of the .h#
classes that we can use to set up more consistency throughout the theme. It's not gonna be a priority, but we eventually do need to really squash down the title classes.
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.
Yea I agree it's weird. Thanks for creating the issue. I just assumed it shouldn't have the class title-wrapper-with-link
when the link is available, but we can tackle it in a different PR
* main: Adjust spacing of elements around pickup availability on product page (Shopify#248) adjust conditional rendering of view all link (Shopify#251) Video section (Shopify#178)
Why are these changes introduced?
Fixes #233
What approach did you take?
This issue only applies to the view all button that displays alongside the title when the mobile slider setting enabled. This element just needed the same conditional logic the other view all button was using (if the number of products visible was less than the number of products in the collection)
Demo links
Checklist