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

Fix slideshow content container borders #1353

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Conversation

andrewetchen
Copy link
Contributor

@andrewetchen andrewetchen commented Feb 11, 2022

Why are these changes introduced?

Fixes #1288

What approach did you take?

Slideshow section

For slideshow section image bocks, when the Show container on desktop setting is unchecked/disabled, the content container shows left and right borders. These borders show up when using global Content container > Border settings

Currently, in the component.slideshow.css, the following rule applies to the slideshow content container when the Grid layout setting is enabled:

slideshow-component.page-width .slideshow__text {
  border-right: var(--text-boxes-border-width) solid rgba(var(--color-foreground), var(--text-boxes-border-opacity));
  border-left: var(--text-boxes-border-width) solid rgba(var(--color-foreground), var(--text-boxes-border-opacity));
}

Wrapping the above in a media query fixes this issue:

@media screen and (max-width: 749px) {
  slideshow-component.page-width .slideshow__text {
    border-right: var(--text-boxes-border-width) solid rgba(var(--color-foreground), var(--text-boxes-border-opacity));
    border-left: var(--text-boxes-border-width) solid rgba(var(--color-foreground), var(--text-boxes-border-opacity));
  }
}
  • Tested with Chrome, FireFox, and Safari

Other considerations

As noted here: #1288 (comment), I initially thought removing the slideshow-component.page-width .slideshow__text rule entirely from component-slideshow.css would fix the issue but I didn't account for left and right borders on mobile, which is being handled here:

dawn/assets/base.css

Lines 2708 to 2711 in ad989e6

@media screen and (max-width: 749px) {
.content-container--full-width-mobile {
border-left: none;
border-right: none;

Testing steps/scenarios

Demo links

Checklist

Copy link

@duygukalaycioglu duygukalaycioglu left a comment

Choose a reason for hiding this comment

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

It would be ideal if there is no auto-rotation on slides as you interact on the slide blocks settings. This ticket has solution ideas for editor only JS fixes. (no impact on performance)

cc: @nicklepine @melissaperreault

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@andrewetchen andrewetchen merged commit ccb5c07 into main Feb 22, 2022
@andrewetchen andrewetchen deleted the fix-slideshow-borders branch February 22, 2022 14:54
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
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.

[Slideshow] Borders remain when container is no longer visible.
4 participants