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.

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(news): rewrite the carousel and improve ux #1513

Merged
merged 22 commits into from
Jun 30, 2023

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Apr 24, 2023

This PR rewrites the news carousel on the home page in an effort to make numerous UX improvements and bug fixes.

Why?
The current news carousel is implemented using a legacy jQuery rcarousel component. rcarousel has not received an update in almost 10 years. It has a large bundle size, is tethered to jQuery, is not responsive, and is not mobile-friendly. It was an impressive library for its time, but it unfortunately does not leverage features of the modern web.

There are numerous bugs in its implementation. Two that come to mind:

  1. Because rcarousel requires the component's structure to be managed by client-side code, there is a flash of unstructured content that appears on the home page on first load for every user with a modest connection speed. The duration of this flash is at the mercy of the user's network speed.

  1. Content is truncated, and sometimes unreadable, on devices utilizing the XS, SM, and MD breakpoints. This notably includes all mobile phone users and most tablet users. This is likely because rcarousel was written before mobile browsers were all that capable.

I know at some point there is a desire to move away from a carousel for news entirely. However until that can happen, it makes sense to enable this feature to be the best it can possibly be.


How?

The carousel has been rewritten to be fully implemented in Blade, Tailwind, Alpine.js, and TypeScript. Additionally, the implementation has integration test coverage.

High level UX highlights:

  • There is no longer a flash of unstyled content in the carousel itself. A separate flash does remain from a bug in the page layout, but the carousel is no longer exacerbating the issue.
  • The giant navigation buttons have been shrunk down to hang along the carousel sides.
  • The background image is now full-width and responsive.
  • Hovering on a news item darkens and slightly blurs the image to improve readability.
  • Hovering on a news item pins the carousel in place - it will not autoscroll while hovered.
  • To improve perceived performance, the first item in the carousel no longer has a text animation.
  • Text animations are completely disabled on mobile.
  • Mobile users can now swipe to advance through the carousel (!)
  • Content is no longer truncated on mobile devices, including down to the iPhone SE.
  • Touch events will now pin the carousel in place, preventing auto-scrolls from occurring.
  • The carousel will no longer auto-scroll when the user tabs away from the page.

Implementation highlights:

  • The structure of the carousel is pure CSS and driven by the scroll snap API.
  • I have renamed the component in the news folder to carousel-2.blade.php. This was done because I figured if the filename stayed the same, the diff would make no sense. I am happy to change it back though if that is preferred.

Working.Autoscroll.mp4
Desktop.Hover.Effect.mp4
Carousel.Manual.Navigate.mp4
Mobile.Swiping.mp4

@wescopeland wescopeland requested review from Tsearo and EmoonX May 10, 2023 20:31
@Jamiras
Copy link
Member

Jamiras commented May 25, 2023

Minor: Clicking on a specific page causes the indicator for that page to remain selected during autoscroll:
image

Minor: Resizing the window resets the carousel to the first page.

@wescopeland
Copy link
Member Author

Minor: Clicking on a specific page causes the indicator for that page to remain selected during autoscroll: image

Nice catch, addressed in 77ace74.

Minor: Resizing the window resets the carousel to the first page.

Intentional -- I can fix this if it's deemed a showstopper, but this behavior was intentionally coded in the TypeScript implementation because of the nature of the CSS scroll snap API. Because the entire carousel lives on a dynamically sized "track" (div), the user's current slide is easily lost when resizing the browser window. To avoid this, it's reset to the first index on resize.

The problem is solvable, but not without adding a lot of client-side complexity.

@Jamiras
Copy link
Member

Jamiras commented May 28, 2023

At unreasonably small window sizes, a horizontal scrollbar appears that you can use to jump between slides without updating the selected slide.

image

@Jamiras
Copy link
Member

Jamiras commented May 28, 2023

Minor: Resizing the window resets the carousel to the first page.

The problem is solvable, but not without adding a lot of client-side complexity.

That's fine. I was just noting the change in behavior.

Copy link
Member

@Jamiras Jamiras left a comment

Choose a reason for hiding this comment

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

Code is functional. Noted a couple of minor things.

Would like to get @luchaos's approval on the implementation.

@wescopeland
Copy link
Member Author

@luchaos @Jamiras This branch has been updated to resolve all outstanding conflicts with V3.

@wescopeland wescopeland requested a review from Jamiras June 30, 2023 13:01
@Jamiras Jamiras merged commit ad7521e into RetroAchievements:master Jun 30, 2023
@wescopeland wescopeland deleted the tailwind-carousel branch June 30, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants