-
Notifications
You must be signed in to change notification settings - Fork 0
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
🏗️ Feat / implement tile slider #175
base: develop
Are you sure you want to change the base?
Conversation
43b5f6d
to
290e280
Compare
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 have tested this on Android and I noticed two things:
*The last one is unrelated to your change, but has been introduced somewhere in the last 27 commits. Is this done with intention? These are just something I noted at first glance. I haven't done a throughout UX/a11y deep-dive. Also, based on experience I think the failed e2e tests take somewhere around 4 hours to fix. I could help you with fixing these issues, but I'm afraid they won't make our upcoming release. |
Thanks for testing! I think I've fixed all feedback :-) Let's wait for the status checks... |
This was caused by a
The prev button is disabled before sliding. The next button is focusable, but only after going through the list first.
I've applied the same fix using
This was caused by a test
These are fixed (luckily it didn't took 4 hours 😄) |
> | ||
const renderRightControl: RenderControl = useCallback( | ||
({ onClick, disabled }) => ( | ||
<button className={styles.chevron} disabled={disabled} aria-label={t('slide_next')} onClick={onClick}> |
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.
Applying adisabled
attribute, has the potential of losing focus when the button changes its disabled-state. This was a bug we experienced on Android in a different situation.
Also I noticed you applied styling based on :disabled
. So that needs to be changed too.
While testing this I noticed the contents of renderRightControl
does not get rendered at all when there isn't a next slide. Causing some weird behaviour (focus is moved to different Tileslider elements)
Aha. In that case the
For some reason I do not understand, I don't see the |
&:disabled { | ||
opacity: 0.3; | ||
pointer-events: none; |
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.
Currently the disabled attribute and class is not present anymore. So this is unused CSS.
Should we reintroduce these classes? Previously it was previously only present for the prev-button (and as you said; it caused e2e failures).
bddfdb3
to
e738300
Compare
For bookkeeping; I've removed the The slide left button should be disabled when the user hasn't slid, but some e2e tests slide left immediately to reach a particular media item. In this PR I accidentally fixed this because this wasn't working properly (only disabled styling was applied). Instead of updating all e2e tests, I chose to remove the disabled attribute and keep it as is for now... |
This PR is pending for some upcoming tweaks to the tile-slider package |
We need to upgrade again after Videodock/tile-slider#48 is merged |
e738300
to
3d8d350
Compare
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.
LGTM!
3d8d350
to
a997121
Compare
feat: implement tile-slider dependency
a997121
to
16595bc
Compare
Updated with rc6 |
Concluded on our testing day sprint 5: We like where this update is going, but it's not quite there yet (rc6). Therefor we revert back to rc3 (tested in sprint4), take this into our release, and put this PR back in draft in order to make a better tile-slider rc. |
Description
This PR replaces the TileDock component with the @videodock/tile-slider dependency.
See more information here: Videodock/tile-slider#42