Fix improper image size for the collapsible content #2507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kmeleta
requested review from
martinamarien and
kmeleta
and removed request for
martinamarien
kmeleta
reviewed
Apr 21, 2023
ludoboludo
approved these changes
Apr 21, 2023
| | image_tag: | ||
| loading: 'lazy', | ||
| sizes: sizes, | ||
| widths: '50, 75, 100, 150, 200, 300, 400, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3200' |
There was a problem hiding this comment.
I'm not sure it makes sense to add that many extra sizes 🤔 Though it doesn't have a negative impact either.
The only time it's going to be useful is if someone uses the section and removed the collapsible blocks.
There was a problem hiding this comment.
That's fair what you say. I added the same bunch of numbers as we did for other sections, just because I want to make it into a snippet one day, even though some sections don't require that range.
kmeleta
approved these changes
May 1, 2023
sections/collapsible-content.liquid
Outdated
| if section.settings.layout == 'section' | ||
| assign padding_multiplier = 2 | ||
| endif | ||
| assign desktop_tablet_padding = padding_multiplier | times: 100 | append: 'px' |
There was a problem hiding this comment.
Nitpick, but might read a little better if you did 100 | times: multiplier. Non-issue though if you don't agree. Otherwise I like it.
phapsidesGT
pushed a commit
to Gravytrain-UK/gt-shopify-dawn-theme
that referenced
this pull request
Sep 3, 2024
* Fix improper image size for collapsible content * Refactoring and renaming variables. * Fix prettier issue. * Minor refactor.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary:
This PR improves the way images are loaded in the collapsible content section.
Why are these changes introduced?
Fixes #1944.
What approach did you take?
I rewrote the logic of how we create our srcset and choose sizes.
Other considerations
Decision log
Visual impact on existing themes
Testing steps/scenarios
Demo links
Checklist