-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Slideshow section #840
Slideshow section #840
Conversation
@ludoboludo Looking good. I left a few notes for an initial review.
No, since they're
Consider switching to "Slideshow description"
I agree the bullet and number control sizing could be increased. I doesn't need to be the |
@ludoboludo I think after you make these adjustments, I'll setup a few sessions with Fable to get user feedback. I'll report back with any new notes. If you need to merge before I get this feedback, go for it. Just be ready to jump back in if I get high severity issues reported back. |
Thank you so much Ludo! Great work! Made a first pass, will do another round later! UX Update on the remaining points I needed to get back to you + some feedback:
Slider visual controls
Content
Answers to your questions/considerations
I am not sure I understand well the expectations for the focus on the slide. Buyers can click/tap dots or number stepper but it is not the primary way to navigate the slides. The arrows are the main controls. Those main control should have the 44px safe tap target, but not the secondary pagination. They give us a hint of where we are mainly but could be navigated to get to a specific slide. Remaining things that I need to review
|
locales/en.default.schema.json
Outdated
"label": "Grid" | ||
}, | ||
"options__3": { | ||
"label": "Grid and peek" |
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.
"label": "Grid and peek" | |
"label": "Grid with a peek" |
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.
👍 if I do this layout in a follow up PR I might remove it from the settings for now 🤔
@ludoboludo The toggle of the play/pause control on hover/focus is not required. I understand the purpose but as a user navigates through the content the flipping of the icon is very distracting. The end result may be a disoriented user on whether the slideshow is/was paused or playing. |
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.
Just some quick comments. I haven't started my review yet but just caught these by looking at the code.
I think the user intent on hover is to look at the slide, so I would pause immediately on hover, and play again once the mouse is out. For accessibility, I think it is also better to pause immediately – @svinkle can you let me know if this is true? |
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.
Just dumping some initial ideas and questions before the weekend.
Also: you did an amazing work putting up together such a complex section. Thanks again for that . There's still a lot of functionality to test on my end, but from my quick look at the Theme Editor I love what I'm seeing! :)
I'll come back on Monday to finish my review.
assets/global.js
Outdated
this.sliderControlWrapper = this.querySelector('.slider-buttons'); | ||
this.bannerContents = this.slider.querySelectorAll('.banner__content'); |
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.
Would you mind having a:
this.elements = {
// all the variables that you are assigning to a querySelector
}
?
I think we're using this pattern in a few places, but it really helps with legibility. I'm often unsure what is a numeric / boolean variable and what is an element.
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.
Maybe we can take a look together/pair. I feel like some values are also used in SliderComponent
and some are based on conditionals. So curious to see what would make the most sense.
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.
Another option would be to standardize adding Element
or Node
to anything that is an element and the plural for any NodeList
s (querySelectorAll).
I'd probably use Node
because it's shorter and matches the NodeList
object that querySelectorAll
returns.
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.
assets/global.js
Outdated
|
||
this.sliderItemsToShow = Array.from(this.sliderItems).filter(element => element.clientWidth > 0); | ||
this.sliderLastItem = this.sliderItemsToShow[this.sliderItemsToShow.length - 1]; | ||
if( this.sliderItemsToShow.length > 0) this.currentPage = 1; |
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.
Isn't that this.currentPage = 1;
automatically inherited from SliderComponent
initialization? Or are there scenarios where this could be initialized with a different value?
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 thought I needed it because it comes up as undefined when I console log it on initialization. But seems to work fine without.
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.
if( this.sliderItemsToShow.length > 0) this.currentPage = 1; | |
if (this.sliderItemsToShow.length > 0) this.currentPage = 1; |
assets/global.js
Outdated
this.sliderItemsToShow = Array.from(this.sliderItems).filter(element => element.clientWidth > 0); | ||
this.sliderLastItem = this.sliderItemsToShow[this.sliderItemsToShow.length - 1]; |
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.
Correct me if I'm wrong, but isn't that being initialized in initPages
on the SliderComponent
?
Looks like the same code to me.
this.sliderItemsToShow = Array.from(this.sliderItems).filter(element => element.clientWidth > 0);
this.sliderLastItem = this.sliderItemsToShow[this.sliderItemsToShow.length - 1];
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.
It is but if I don't do it there, on initialization I get an error as it sees it as undefined. Im guessing the value is declared/updated in the SliderComponent
right after Im trying to use in the SlideshowComponent
.
@ludoboludo I just noticed a focus management issue. Ex., when you're on the second slide and you activate the "previous slide" control, the control is set as Instead of setting the control as |
I removed I fixed the Dots and Numbers issue - it was broken because of the variable renaming that @LucasLacerdaUX noticed. |
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.
Bugs are fixed. Awaiting UX and a11y approval.
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.
Dots/Numbers are now working as expected 👍
@tyleralsbury Is there a new store link I could test these changes with? |
1e5bf81
} | ||
|
||
@media screen and (max-width: 749px) { | ||
.slider-counter__link { | ||
padding: 0 0.7rem; | ||
padding: 0.7rem; |
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.
Comment from Scott on slack that it's addressing:
Before merging I’d like to see a little more padding on the number/bullet controls. Right now they’re 24 by 10. Could we make them 24 by 24? Same for the numbers; make the height the same as the width to add a better touch target/visual focus.
assets/global.js
Outdated
@@ -692,6 +692,7 @@ class SlideshowComponent extends SliderComponent { | |||
linkToSlide(event) { | |||
event.preventDefault(); | |||
const slideScrollPosition = this.slider.scrollLeft + this.sliderFirstItemNode.clientWidth * (this.sliderControlLinksArray.indexOf(event.currentTarget) + 1 - this.currentPage); | |||
this.slider.setAttribute('aria-live', 'polite'); |
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.
Other comment from Scott I'm addressing:
When the slideshow is set to autoplay and it’s playing, the transition announcement doesn’t happen on number/bullet/arrow press. It does announce after the slideshow is paused from pressing the pause button, arrow/number/bullet press announces the transition.
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.
Looks good to me :)
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.
A monumental task! ✅
a1ebaba
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.
✅
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.
👍
Why are these changes introduced?
Fixes #833
What approach did you take?
I reused quite a bit of the styling for the image banner as if you have only one slide the look is very similar. Section settings are also very similar so it seemed to make sense.
I create a new JS class that extends the functionality of the existing SliderComponent.
Other considerations
Issue: when editing blocks/slides it won't show you the current slide you're editing in the theme editor.
Potential benefits to add:
image-banner.css
file as they share quite a bit of styling. But the code in common could be put into its own file as around 40% of theimage-banner.css
isn't necessary to the slideshow.content-visibility
. How does it behave if used in browser that don't support the property ? Remove lazy loading of the first slide to load faster when it's the first section and rely on content-visibility to delay the loading when it isn't.Demo links
Checklist