-
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 all 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,84 @@ 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--everywhere { | ||
margin-bottom: 0; | ||
} | ||
|
||
.announcement-bar-slider, | ||
.announcement-bar-slider .slider { | ||
width: 100%; | ||
} | ||
ludoboludo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
.announcement-bar .slider-button--next { | ||
margin-right: -1.6rem; | ||
min-width: 44px; | ||
} | ||
|
||
.announcement-bar .slider-button--prev { | ||
margin-left: -1.6rem; | ||
min-width: 44px; | ||
} | ||
|
||
.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)); | ||
} | ||
|
||
@media screen and (min-width: 750px) { | ||
.announcement-bar .slider-button--next { | ||
margin-right: -3.8rem; | ||
} | ||
|
||
.announcement-bar .slider-button--prev { | ||
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; | ||
} | ||
|
||
} | ||
|
||
.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 { | ||
|
@@ -2277,6 +2343,7 @@ product-info .loading-overlay:not(.hidden) ~ *, | |
} | ||
|
||
.announcement-bar__message { | ||
text-align: center; | ||
padding: 1rem 0; | ||
margin: 0; | ||
letter-spacing: 0.1rem; | ||
|
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.