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.

Sign up for GitHub

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

Jump to bottom

Announcements slideshow #2394

Merged
merged 30 commits into from
Apr 4, 2023
Merged

Announcements slideshow #2394

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9c438fd
Add slideshow for announcements
eugenekasimov Mar 14, 2023
ab95fb1
Add full width for announcements slider
eugenekasimov Mar 14, 2023
7798534
Add component-slider.css
eugenekasimov Mar 15, 2023
eea8fe8
Add 100% height for buttons. Vertical aligning for text.
eugenekasimov Mar 15, 2023
b75b672
Fix conflicts
eugenekasimov Mar 15, 2023
e596ae8
Merge branch 'main' into add-announcements-slideshow
eugenekasimov Mar 15, 2023
e2209e5
Add missed class that caused wrong alignment
eugenekasimov Mar 15, 2023
3985d50
Change accessibility attributes from slideshow to announcements
eugenekasimov Mar 16, 2023
410d963
Minor change for aria-label
eugenekasimov Mar 16, 2023
f96e12d
Update en.default.json file
eugenekasimov Mar 17, 2023
006f8a8
Refactor and change naming of classes
eugenekasimov Mar 17, 2023
5452ddc
Add missing role for slideshow-component
eugenekasimov Mar 20, 2023
c8ad9a7
Remove hover for announcement link
eugenekasimov Mar 20, 2023
d0caa81
Refactor css classes. Remove unnecessary chunk of code.
eugenekasimov Mar 23, 2023
a85a5dd
Fix if logic in liquid
eugenekasimov Mar 23, 2023
c0a1d44
Add JS logic for prefers-reduced-motion for both announcements and sl…
eugenekasimov Mar 24, 2023
40da407
Refactoring
eugenekasimov Mar 24, 2023
4c77e36
Cancel preventing auto-rotation from slideshow when reduced-motion. A…
eugenekasimov Mar 28, 2023
8f579e4
Merge branch 'main' into add-announcements-slideshow
eugenekasimov Mar 28, 2023
8687422
Correct focus-visible for announcements slideshow buttons
eugenekasimov Mar 28, 2023
570c23a
Add settings to en.default.schema.json
eugenekasimov Mar 28, 2023
36204d9
Minor refactoring
eugenekasimov Mar 29, 2023
2ec496c
Update 6 translation files
translation-platform[bot] Mar 30, 2023
08e0521
Update 5 translation files
translation-platform[bot] Mar 30, 2023
a7fccc7
Add underline decoration for link
eugenekasimov Mar 30, 2023
0731f8e
Merge branch 'main' into add-announcements-slideshow
eugenekasimov Mar 30, 2023
87a678e
Update 24 translation files
translation-platform[bot] Mar 30, 2023
d2fea77
Update 15 translation files
translation-platform[bot] Mar 30, 2023
e2f0d24
Remove unnecessary class. Move css styles for announcements-slider to…
eugenekasimov Apr 1, 2023
1cc56cc
Add min-width for buttons.
eugenekasimov Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved
max-width: 100%;
}

.page-width.drawer-menu {
max-width: 100%;
}
Expand Down Expand Up @@ -2227,9 +2231,25 @@ product-info .loading-overlay:not(.hidden) ~ *,
}

/* section-announcement-bar */
.announcement-bar {
.announcement-bar--bottom-border {
border-bottom: 0.1rem solid rgba(var(--color-foreground), 0.08);
}

.announcement-bar {
color: rgb(var(--color-foreground));
height: 100%;
display: flex;
justify-content: center;
flex-wrap: wrap;
align-content: center;
}

.announcement-bar-container {
height: 100%;
}
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved

.announcement-bar-slider {
width: 100%;
}

.announcement-bar__link {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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?
Capture d’écran, le 2023-03-30 à 14 22 58

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

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

Cool then! @eugenekasimov would that be easy to add this state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YoannJailin done ✅

Expand Down Expand Up @@ -2257,6 +2277,7 @@ product-info .loading-overlay:not(.hidden) ~ *,
}

.announcement-bar__message {
text-align: center;
padding: 1rem 0;
margin: 0;
letter-spacing: 0.1rem;
Expand Down
41 changes: 39 additions & 2 deletions assets/component-slider.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -359,6 +363,39 @@ slider-component.slider-component-full-width {
justify-content: center;
}

.announcement__slider-button--next {
margin-right: -2rem;
height: 100%;
}

.announcement__slider-button--prev {
margin-left: -2rem;
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved
height: 100%;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

}


eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved
@media screen and (min-width: 750px) {
.announcement__slider-button--next {
margin-right: -3.8rem;
}

.announcement__slider-button--prev {
margin-left: -3.8rem;
}

}
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved

@media screen and (min-width: 990px) {
body:has( .section-header .header:not(.drawer-menu)) .announcement-bar-section .announcement__slider-button--next {
margin-right: -2rem;
}

body:has( .section-header .header:not(.drawer-menu)) .announcement-bar-section .announcement__slider-button--prev {
margin-left: -2rem;
}

metamoni marked this conversation as resolved.
Show resolved Hide resolved
}

.slider-button:not([disabled]):hover {
color: rgb(var(--color-foreground));
}
Expand All @@ -372,11 +409,11 @@ slider-component.slider-component-full-width {
cursor: not-allowed;
}

.slider-button--next .icon {
.slider-button--next .icon, .announcement__slider-button--next .icon{
transform: rotate(-90deg);
}

.slider-button--prev .icon {
.slider-button--prev .icon, .announcement__slider-button--prev .icon{
transform: rotate(90deg);
}

Expand Down
17 changes: 8 additions & 9 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,17 +679,16 @@ class SlideshowComponent extends SliderComponent {
}

setAutoPlay() {
this.sliderAutoplayButton = this.querySelector('.slideshow__autoplay');
if (this.querySelector('.slideshow__autoplay') !== null) {
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved
this.sliderAutoplayButton.addEventListener('click', this.autoPlayToggle.bind(this));
this.addEventListener('mouseover', this.focusInHandling.bind(this));
this.addEventListener('mouseleave', this.focusOutHandling.bind(this));
this.addEventListener('focusin', this.focusInHandling.bind(this));
this.addEventListener('focusout', this.focusOutHandling.bind(this));
this.autoplayButtonIsSetToPlay = true;
}
kmeleta marked this conversation as resolved.
Show resolved Hide resolved
this.autoplaySpeed = this.slider.dataset.speed * 1000;

this.sliderAutoplayButton.addEventListener('click', this.autoPlayToggle.bind(this));
this.addEventListener('mouseover', this.focusInHandling.bind(this));
this.addEventListener('mouseleave', this.focusOutHandling.bind(this));
this.addEventListener('focusin', this.focusInHandling.bind(this));
this.addEventListener('focusout', this.focusOutHandling.bind(this));

this.play();
this.autoplayButtonIsSetToPlay = true;
}

onButtonClick(event) {
Expand Down
7 changes: 7 additions & 0 deletions locales/en.default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@
}
},
"sections": {
"announcements": {
"previous_announcement": "Previous announcement",
"next_announcement": "Next announcement",
"carousel": "Carousel",
"announcement": "Announcement"
},

"header": {
"announcement": "Announcement",
"menu": "Menu",
Expand Down
Loading