-
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
Changes from 28 commits
9c438fd
ab95fb1
7798534
eea8fe8
b75b672
e596ae8
e2209e5
3985d50
410d963
f96e12d
006f8a8
5452ddc
c8ad9a7
d0caa81
a85a5dd
c0a1d44
40da407
4c77e36
8f579e4
8687422
570c23a
36204d9
2ec496c
08e0521
a7fccc7
0731f8e
87a678e
d2fea77
e2f0d24
1cc56cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -249,6 +249,10 @@ html.no-js .no-js-hidden { | |
padding: 0 1.5rem; | ||
} | ||
|
||
body:has(.section-header .drawer-menu) .announcement-bar-section slideshow-component { | ||
max-width: 100%; | ||
} | ||
|
||
.page-width.drawer-menu { | ||
max-width: 100%; | ||
} | ||
|
@@ -2246,22 +2250,43 @@ product-info .loading-overlay:not(.hidden) ~ *, | |
line-height: calc(1 + 0.1 / var(--font-body-scale)); | ||
} | ||
|
||
/* section-announcement-bar */ | ||
.announcement-bar { | ||
/* utility-bar */ | ||
.utility-bar { | ||
height: 100%; | ||
} | ||
|
||
.utility-bar--bottom-border { | ||
border-bottom: 0.1rem solid rgba(var(--color-foreground), 0.08); | ||
} | ||
|
||
.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; | ||
} | ||
Comment on lines
+2262
to
+2271
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
.announcement-bar-slider, | ||
.announcement-bar-slider .slider { | ||
width: 100%; | ||
} | ||
|
||
.announcement-bar__link { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm aligned with the decision to remove the background hover effect, but now not sure if the arrow animation alone is enough feedback to visually indicate the hover state. I wonder if it might be worth reducing the initial opacity of the text and changing to 100% on hover, like many other links do (example) Not sure if this was considered or not, but wanted to run it by @YoannJailin just in case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey! Agree an additional effect could help. Did some testing, the only matter i see with opacity is that some announcements may be clickable, some others not. Might look strange to switch between annoucements more or less transparent depending of they are clickable or not. Is it worth considering a line on hover when it's clickable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True true, good point. I don't mind the underline myself and we do it for many other elements including the nearby L1 nav items. I'll defer to your judgement though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool then! @eugenekasimov would that be easy to add this state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @YoannJailin done ✅ |
||
display: block; | ||
display: flex; | ||
width: 100%; | ||
padding: 1rem 0; | ||
text-decoration: none; | ||
height: 100%; | ||
justify-content: center; | ||
align-items: center; | ||
} | ||
|
||
.announcement-bar__link:hover { | ||
color: rgb(var(--color-foreground)); | ||
background-color: rgba(var(--color-card-hover), 0.06); | ||
text-decoration: underline; | ||
} | ||
|
||
.announcement-bar__link .icon-arrow { | ||
|
@@ -2276,7 +2301,9 @@ product-info .loading-overlay:not(.hidden) ~ *, | |
padding: 0; | ||
} | ||
|
||
.announcement-bar__message { | ||
.announcement-bar__message, | ||
.announcement-bar-slider__message { | ||
eugenekasimov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
text-align: center; | ||
padding: 1rem 0; | ||
margin: 0; | ||
letter-spacing: 0.1rem; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,6 +135,10 @@ slider-component.slider-component-full-width { | |
scroll-snap-align: center; | ||
} | ||
|
||
.announcement-bar .slider--everywhere { | ||
margin-bottom: 0; | ||
} | ||
|
||
@media screen and (min-width: 990px) { | ||
.slider-component-desktop.page-width { | ||
max-width: none; | ||
|
@@ -359,6 +363,43 @@ slider-component.slider-component-full-width { | |
justify-content: center; | ||
} | ||
|
||
.announcement-bar .slider-button--next { | ||
margin-right: -1.6rem; | ||
height: 100%; | ||
} | ||
|
||
.announcement-bar .slider-button--prev { | ||
margin-left: -1.6rem; | ||
height: 100%; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
.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 commentThe 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 |
||
|
||
@media screen and (min-width: 750px) { | ||
.announcement-bar .slider-button--next { | ||
margin-right: -3.8rem; | ||
} | ||
|
||
.announcement-bar .slider-button--prev { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
margin-left: -3.8rem; | ||
} | ||
} | ||
|
||
@media screen and (min-width: 990px) { | ||
body:has(.section-header .header:not(.drawer-menu)) .announcement-bar-section .announcement-bar .slider-button--next { | ||
margin-right: -1.8rem; | ||
} | ||
|
||
body:has(.section-header .header:not(.drawer-menu)) .announcement-bar-section .announcement-bar .slider-button--prev { | ||
margin-left: -1.8rem; | ||
} | ||
|
||
metamoni marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
.slider-button:not([disabled]):hover { | ||
color: rgb(var(--color-foreground)); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -477,6 +477,14 @@ | |
}, | ||
"announcement-bar": { | ||
"name": "Announcement bar", | ||
"settings": { | ||
"auto_rotate": { | ||
"label": "Auto-rotate slides" | ||
}, | ||
"change_slides_speed": { | ||
"label": "Change slides every" | ||
} | ||
}, | ||
Comment on lines
+480
to
+487
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just calling out that I agree with duplicating these particular translations for the announcement bar vs reusing the entries from slideshow. These are not particularly verbose and even though slideshow code is being used as a layer of abstraction here, slideshow is a mostly-unrelated, fully independent section on its own and I don't know if there's a strong case to rely on its settings here. |
||
"blocks": { | ||
"announcement": { | ||
"name": "Announcement", | ||
|
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.