-
-
Notifications
You must be signed in to change notification settings - Fork 476
feat(carousel): automatic hiding of navigation buttons when the first… #1207
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
base: main
Are you sure you want to change the base?
feat(carousel): automatic hiding of navigation buttons when the first… #1207
Conversation
… or last image is active Added 'autoHideControls' feature to ImageSlider for intuitive navigation button visibility.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1207 +/- ##
==========================================
- Coverage 99.54% 97.35% -2.19%
==========================================
Files 163 214 +51
Lines 6621 9089 +2468
Branches 401 532 +131
==========================================
+ Hits 6591 8849 +2258
- Misses 30 240 +210 ☔ View full report in Codecov by Sentry. |
|
Thank you for contributing! I like this idea and solution a lot, but like you said, it's not in the vanilla library so we probably shouldn't add it. @zoltanszogyenyi What do you think here? This PR looks good if we want it. |
| className={`${theme.root.rightControl} transition-opacity duration-300 ${ | ||
| activeItem === items.length - 1 && autoHideControls ? 'pointer-events-none opacity-0' : 'opacity-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.
Thanks for your contribution.
Would opacity be the best approach in this case? W/opacity the controls will remain available to user interactions on the dom, while display:none would prevent interactions.
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.
Good catch, yes, that's right.
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. If with controls remaining available you mean that the user will go from last image to first image even with control hided, in my tests when control hides click has no effect. In this video you will see a circle in the pointer when I click:
Grabacion.de.pantalla.2024-01-01.a.la.s.1.30.21.p.m.mov
Please correct me if I misunderstood.
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 would still change this to display:none because this may come up as still clickable for users navigating with Tab.
I can't confirm that since you didn't add an example to the docs, which, if you could, would be great.
With that said, I'm still not sure if we can merge this because this is a UX feature that isn't available in the vanilla library at all.
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.
You are right! Thanks for your feedback I will implement this in my forked repo. Please let me know if you'd like this feature here so I can update the PR.
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.
Keep in mind that if the navigation button/icon disappears from the DOM, when the user spam clicks next (or prev) until he gets to the last, if in the future we'll implement click-to-fullscreen (or similar), then this behaviour would be very unpredictable.
So the case study is, the next/prev icons are wrapped in a button element in order to be focusable/accessible and the button becomes disabled=true when is last or first page, according to the direction, this way tabbing is not possible (CC @tulup-conner ) and also clicking the disabled button should stop event propagation.
#just_thoughts 🤔
Avoid use of controls when tab pressed
Added example of autoHideControls for carousel.
ran prettier to format the code
feat(carousel): automatic hiding of navigation buttons when the first or last image is active
Added 'autoHideControls' feature to ImageSlider for intuitive navigation button visibility.
Gives a nice touch for UX when not needing to move from last image to first one.
Conditional rendering of carousel controls when autoHideControls is true.
demo.mp4
fixes #1205
New optional prop added to Carousel component: autoHideControls with false as default value.

Example: