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

Feat / Featured shelf cosmetic update #628

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Feat / Featured shelf cosmetic update #628

merged 1 commit into from
Nov 1, 2024

Conversation

royschut
Copy link
Collaborator

@royschut royschut commented Oct 23, 2024 •
edited
Loading

Feat / Featured shelf cosmetic update

This PR improves the look and feel of the featured shelf. Instead of using the TileSlider with just a larger Card, this new version leverages the reserved space much more, with a smooth animation between slides. On mobile devices, you can drag the text off screen to bring in the next.

See the screenshots/screen caps of desktop and mobile below.

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

Screenshot 2024-10-25 at 10 24 44
Screenshot 2024-10-25 at 10 24 21

desktop.mov
mobile.MP4

@royschut royschut force-pushed the feat/featured-shelf-update branch from 5b1b076 to 77582c4 Compare October 23, 2024 16:24
@langemike
Copy link
Collaborator

langemike commented Oct 24, 2024 •
edited
Loading

Really nice improvement! I detected one visual problem; when you use the pagination dots, they move. So i need to reposition my mouse cursor anytime I want to click the dots.

Other points to consider:

  1. Curious: why have you chosen to do inline JS/CSS animations as opposed to regular CSS? I think the later makes it easier to make styling changes. I understand it to calculate a dot size, but maybe the rest could be done in regular CSS.
  2. I think we should use aria-disabled instead of disabled on the slider buttons/paginations. So the buttons can still keep their focus, while the screen-reader pronounces it's state. I know this is not the first time we have discussions about this ;-)
  3. I think we should wrap the pagination within <nav> and <ul>. So the screen-reader pronounces the amount of items and makes it easier to navigate.

@royschut royschut force-pushed the feat/featured-shelf-update branch 3 times, most recently from 14e8ebc to 40b5358 Compare October 25, 2024 08:21
@royschut royschut marked this pull request as ready for review October 25, 2024 11:39
@royschut royschut requested a review from langemike October 28, 2024 14:47
langemike
langemike approved these changes Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@langemike langemike left a comment

Choose a reason for hiding this comment

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

Nice work! I'm satisfied with the applied height. I think the pagination alignment can be improved, but this is something for in the future to iterate on. Good enough for now!

@royschut royschut added the JWP Audit This PR needs a review from an JWP employee label Oct 30, 2024
@royschut
Copy link
Collaborator Author

Hi @AntonLantukh and @dbudzins, I hope you find a moment to have a look as well. :-)

@ChristiaanScheermeijer ChristiaanScheermeijer mentioned this pull request Oct 30, 2024
Merged
@royschut royschut force-pushed the feat/featured-shelf-update branch from 483b694 to 9266168 Compare November 1, 2024 13:38
@royschut
feat(home): add jumbotron featured shelf
c2f9f1f 
refactor(home): code splitting

refactor(home): more code splitting

feat(a11y): make new featured shelf accessible

feat(home): make featured pagination acessible

feat(home): add swipable mobile featured shelf

refactor(home): clean up code

refactor(home): minor improvements

refactor(home): code optimisations

refactor(home): minor style improvements

refactor(home): minor ui improvements

refactor(home): reduce mobile shelf height

refactor(home): improve scroll/swipe locking

refactor(home): improve swipe area and target

refactor(home): fix pagination not always clickable

refactor(home): reposition chevrons and later fade out mobile

refactor(home): allow 2 lines title

refactor(home): copy update

refactor(home): use nav and li for pagination

refactor(home): animation optimisation

refactor(home): animation optimisation

refactor(home): prevent animation flashing

refactor(home): optimize mobile animation

refactor(home): remove obsolete code

chore(e2e): update tests for new featured shelf

refactor(home): remove obsolete code

refactor(home): minor ui tweaks

refactor(e2e): add minor delay to flaky test

refactor(home): make shelf 70& height of viewport

refactor(home): use variable for header height calculations

Co-authored-by: Christiaan Scheermeijer <christiaan@videodock.com>

refactor(home): dont show start watching btn for non playable type

refactor(home): preload correct item for multi page slide

chore: alignment with video details screen

fix: decrease min height to prevent overlap

refactor: responsive fixes for featured shelf
@royschut royschut force-pushed the feat/featured-shelf-update branch from 9266168 to c2f9f1f Compare November 1, 2024 13:38
@ChristiaanScheermeijer ChristiaanScheermeijer merged commit 08bf7c0 into jwplayer:develop Nov 1, 2024
9 checks passed
@ChristiaanScheermeijer ChristiaanScheermeijer deleted the feat/featured-shelf-update branch November 1, 2024 13:53
@dbudzins dbudzins mentioned this pull request Nov 8, 2024
Closed
@dbudzins dbudzins mentioned this pull request Nov 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@langemike langemike langemike approved these changes

@ChristiaanScheermeijer ChristiaanScheermeijer ChristiaanScheermeijer approved these changes

@AntonLantukh AntonLantukh Awaiting requested review from AntonLantukh AntonLantukh is a code owner

@dbudzins dbudzins Awaiting requested review from dbudzins dbudzins is a code owner

@CarinaDraganJW CarinaDraganJW Awaiting requested review from CarinaDraganJW

Assignees
No one assigned
Labels
JWP Audit This PR needs a review from an JWP employee
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@royschut @langemike @ChristiaanScheermeijer