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

Add padding to collection banner section #1808

Closed
wants to merge 1 commit into from

Conversation

andrewetchen
Copy link
Contributor

This is draft PR to show an alternative solution for: #1790

Store demo link

Testing steps/scenarios

  • Layout > Page width has been set to 1600px:
    • Test by adjusting these values.
  • Media > Shadow > Horizontal and Vertical offsets have been set to the max negative setting:
    • Test by adjusting these values.
  • Navigate to any collection and enable/disable the "Show collection image" setting.
    • Go to the Collection list page and click on the 13 products collection as I was using this for testing.
    • Adjust the top and bottom section padding.

Notes for this draft PR

(We should probably fix this in another PR)

While I was testing the removal of:

padding: calc(4rem + var(--page-width-margin)) 0
calc(4rem + var(--page-width-margin));

I realized the condition of the --page-width-margin CSS variable in layout > password.liquid, theme.liquid, and gift_card.liquid didn't work correctly due to 1600 being wrapper in single quotes (not sure if this was a recent change in the backend):

--page-width-margin: {% if settings.page_width == '1600' %}2{% else %}0{% endif %}rem;

So when you set the global settings > Layout > Page width to 1600px, the CSS variable of --page-width-margin was still 0rem and not the expected 2rem. This still needs to work since this PR uses the same CSS but moved into the section file of main-collection-banner.liquid.

TL;DR - for this draft PR, I removed those single quotes and validated that it works as expected:


CC: @kjellr

@andrewetchen andrewetchen deleted the collection-banner-padding-draft branch February 3, 2023 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant