-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Announcements slideshow #2394
Announcements slideshow #2394
Conversation
When hovering one announcement that has a link, would that be possible to extend the color background change so it affects the whole section background? The current effect cover the text only but not the background behind the arrows. Also when one announcement in longer, the shorter one has that kind of hover : |
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.
Behavior is looking good so far, but mostly did a quick first pass on code. Let me know if any of my feedback is confusing or you disagree with it.
Hey @YoannJailin
I fixed this 👆 |
@YoannJailin and I agreed to remove the hover effect for the announcement links background. c8ad9a7 |
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.
Seems like it's working beautifully to me 👍 Thanks for taking the time to consider all the heavily opinionated code nitpicks.
.announcement-bar, | ||
.announcement-bar__announcement { | ||
color: rgb(var(--color-foreground)); | ||
width: 100%; | ||
height: 100%; | ||
display: flex; | ||
justify-content: center; | ||
flex-wrap: wrap; | ||
align-content: center; | ||
} |
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.
I'm not sure we need all these styles applied to both of these classes. I know we've moved some classes around, so I feel like some of these are redundant now. I should have flagged this sooner though and I don't want to send us backward. If we feel we can confidently clean some of this up that'd be great but I won't block this PR for it.
assets/component-slider.css
Outdated
margin-right: -3.8rem; | ||
} | ||
|
||
.announcement-bar .slider-button--prev { |
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.
Not something we need to address here or at all, just thinking aloud in case it appealed to anyone. With the arrows kind of serving as a frame, it might be a nice little improvement if the boundaries of the slider area had a little fade out/gradient overlay so the scrolling content doesn't just disappear at a harsh line. Something similar may have come up before though (maybe during slideshow development) and I know we didn't roll with that.
assets/component-slider.css
Outdated
.announcement-bar .slider-button--next { | ||
margin-right: -1.6rem; | ||
height: 100%; | ||
} | ||
|
||
.announcement-bar .slider-button--prev { | ||
margin-left: -1.6rem; | ||
height: 100%; | ||
} | ||
|
||
.announcement-bar .slider-button--next:focus-visible, | ||
.announcement-bar .slider-button--prev:focus-visible { | ||
outline-offset: -0.3rem; | ||
box-shadow: 0 0 0 -0.2rem rgb(var(--color-foreground)); | ||
} |
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.
How do we feel about having announcement specific styling in this general slider file ?
Usually feature specific code doesn't go into the general styling file used by other features. For example, product page specific slider code is in section-main-product.css
and not in component-slider.css
🤔
"default": "accent-1", | ||
"label": "t:sections.all.colors.label" | ||
}, | ||
{ |
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.
Here I think we might have issues during theme upgrades as we're moving a setting from a block level to a section level 🤔
Could be worth checking/confirming with the team that could help us have a fix for it or we can test a theme upgrade ourselves and see how it behaves.
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.
Ludo and I have come to an agreement that we simply need to observe what happens after upgrading with announcements, and from that, we will determine if a warning about manual update settings for merchants is necessary.
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.
So I tested a manual update and the experience isn't too bad.
Things you're losing:
- the color scheme (moved to a section level setting instead of block)
- the text alignment, the setting was removed
But otherwise the blocks are keeping the links they had and coming up as a slider as expected.
So we will still need to warn merchants when upgrading but it shouldn't be too bad of an experience.
color: rgb(var(--color-foreground)); | ||
width: 100%; |
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.
Not a huge fan of how far the arrows are from the content: video
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.
Hey Ludo, thanks for the opinion. I'm afraid there is no optimal way to set it right, because it really depends on the message size. Setting the width for the slider based on the first slide might have the same visual issue if the first slide is wide and the next is super narrow. I think the current design decision works well for the average case, plus a merchant can always set the menu to a different type such as the dropdown to make it more narrow. Plus, we are about to start adding other features into it and it's going to be more compact anyway.
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.
Hey @YoannJailin this needs your attention. Thanks.
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.
Hello @eugenekasimov @ludoboludo i think it could be good to make the arrow icons more centered so the user can see them more easily / he won't have to go too far to hover it on large screens.
Let's just add the additional options for now (selectors, social media icons) that will shrink the Announcements slider to the middle including the arrows. We can still make that narrow version of the slider default later, even if there's no other options activated.
assets/component-slider.css
Outdated
|
||
.announcement-bar .slider-button--prev { | ||
margin-left: -1.6rem; | ||
height: 100%; |
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.
Tweaking the height seem to mess with the size of the arrow icons. They should be 44px by 44px ideally to keep them accessible and easy to click on.
If the announcement is a link, on mobile it can be very easy to click on the link when you meant to click on the arrow.
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.
I had to proceed this way because Yoann wanted me to keep the default height of the announcement bar the same as it used to be. If we set a fix height for the buttons 44px then the minimal height of the announcement bar will be bigger.
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.
We gotta do what's best for the user experience. So we, at least for mobile, need to have buttons be 44 by 44.
cc: @YoannJailin
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.
@YoannJailin after discussing it with Ludo, I have to admit that his argument is quite valuable and important. I remember that we discussed it with you and you wanted to keep the default height of the announcement bar the same as it used to be. But because we added arrow buttons and there is a requirement for accessibility we should set those buttons at least 44px height and the announcement bar is going to have a minimum height of 44px as well.
… base.css. Refactor JS.
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.
Still looks good to me with the latest changes
Exciting team! I was just looking at the experience on our main branch and came here to share some observations. I'd like to raise a few key points for a follow up refinement for accessibility and usability before our next release.
|
* shopify/main: [Video] Add fade in on scroll animation (Shopify#2495) Animate rich text and email signup (Shopify#2408) [Gift card] Add help doc link and change label for translation (Shopify#2471) Add prettier config to support format-on-save (Shopify#2323) Announcements slideshow (Shopify#2394) fix: Update Follow on Shop Option Text (Shopify#2463) Enable "per-PR" async PR mode (Shopify#2488) Animate multicolumn (Shopify#2409) Fix improper image sizes in the collage section (Shopify#2478) Replaced depreciated liquid property/value (Shopify#2480)
* next: [Video] Add fade in on scroll animation (Shopify#2495) Animate rich text and email signup (Shopify#2408) [Gift card] Add help doc link and change label for translation (Shopify#2471) Add prettier config to support format-on-save (Shopify#2323) Announcements slideshow (Shopify#2394) fix: Update Follow on Shop Option Text (Shopify#2463) Enable "per-PR" async PR mode (Shopify#2488) Animate multicolumn (Shopify#2409) Fix improper image sizes in the collage section (Shopify#2478) Replaced depreciated liquid property/value (Shopify#2480) Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next
* Add slideshow for announcements * Add full width for announcements slider * Add component-slider.css * Add 100% height for buttons. Vertical aligning for text. * Fix conflicts * Add missed class that caused wrong alignment * Change accessibility attributes from slideshow to announcements * Minor change for aria-label * Update en.default.json file * Refactor and change naming of classes * Add missing role for slideshow-component * Remove hover for announcement link * Refactor css classes. Remove unnecessary chunk of code. * Fix if logic in liquid * Add JS logic for prefers-reduced-motion for both announcements and slideshow component * Refactoring * Cancel preventing auto-rotation from slideshow when reduced-motion. Add mouse over/leave and focusin/focusout to stop rotation * Correct focus-visible for announcements slideshow buttons * Add settings to en.default.schema.json * Minor refactoring * Update 6 translation files * Update 5 translation files * Add underline decoration for link * Update 24 translation files * Update 15 translation files * Remove unnecessary class. Move css styles for announcements-slider to base.css. Refactor JS. * Add min-width for buttons. --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
PR Summary:
This PR changes the way announcement blocks are displayed. Instead of being stacked under each other now they are displayed as a slideshow.
The width of the slider is aligned with the width of the header.
Why are these changes introduced?
Fixes #398.
What approach did you take?
Other considerations
To make the slider follow the width of the header I had to use quite complex combinations of css selectors. Although it works, maybe it's not worth it and we could use JS to achieve the same result 🤔.
Decision log
Visual impact on existing themes
Testing steps/scenarios
Demo links
Checklist