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

[Blog post section] Fix slides size on mobile #2368

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Mar 7, 2023

PR Summary:

This fixes the slider for the blog post section on mobile that doesn't show the right amount of total slides.

Why are these changes introduced?

Fixes https://github.com/Shopify/dawn-private/issues/141

What approach did you take?

There are two approached here that could be taken:

  1. We overwrite the spacing in between each slide. Since right now it's a full width slide, we can force a gap value instead of using the one from the general settings (horizontal space).
  2. We can reuse the same approach we have on the product page where the width isn't 100% - spacing but calc(100% - 3rem - var(--grid-mobile-horizontal-spacing)). This is great as well but has a visual impact. As it adds a peek-a-boo effect that wasn't there before:
Screenshot comparison

In this first iteration of the PR I went with approach 1.
Updated it so it uses approach 2 👍

Other considerations

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

None with option 1 and if we go with option 2, I shared a screenshot with the visual impact, adding the peek a boo effect which wasn't there before on mobile.

Testing steps/scenarios

  • Add a blog post section on any template,
  • Go into the general layout settings and play with the value of horizontal spacing,
  • Make sure the total pages is always calculated properly on mobile

Demo links

Checklist

@ludoboludo ludoboludo changed the title [Blog post section] Overwrite spacing on mobile [Blog post section] Fix slides size on mobile Mar 13, 2023
@kmeleta kmeleta self-requested a review March 16, 2023 16:25
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Approach 2 seems like the right approach here. The slider has the grid--peek class and I'm sure it was always intended to behave that way and it was just never updated with the variable padding.

Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

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

🚀

@ludoboludo ludoboludo merged commit bcbb9d6 into main Mar 17, 2023
@ludoboludo ludoboludo deleted the blog-section-slider-spacing branch March 17, 2023 14:46
pangloss added a commit to pangloss/dawn that referenced this pull request Mar 23, 2023
* shopify/main:
  Gift cards/add recipient (Shopify#2412)
  Update 1 translation file (Shopify#2453)
  [Slideshow] Add ambient movement to background (Shopify#2383)
  Wrap calls to search results ind collection counts in paginate to reduce additional requests (Shopify#2421)
  [Header] Add app block in the header section (Shopify#2238)
  [Image with Text] Add ambient movement to image (Shopify#2385)
  Update 1 translation file (Shopify#2450)
  [Blog post section] Fix slides size on mobile  (Shopify#2368)
  Duplicated scrollbar in drawer menu (Shopify#2400)
  [Header locales] Support gradients (Shopify#2386)
  [Image banner] Add ambient movement to background (Shopify#2342)
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Overwrite spacing on mobile for blog post section

* switch to option 2
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.

3 participants