AdvisorySG / mentorship-page

React app for Advisory's Mentorship Network page
https://mentorship.advisory.sg/
MIT License
1 stars 14 forks source link

[MPT-110] Fix Testimonial Carousel #784

Closed Hackin7 closed 3 months ago

Hackin7 commented 3 months ago

Initial Issue - Mobile view/ other view bugs out

image

Suspected root cause of the issue is the isSmall variable

const Index = () => {
  const isSmall = useMediaQuery("(max-width: 800px)");
  const glideRef = useRef(null);

When the component is first initialised, the value is ALWAYS false, regardless of orientation. It is then rendered once. This variable is then updated, but the component is not rerendered/ reinitialised after the variable update. The buggy mentor/ mentee listing (and hence offset) would then appear.

The PR initialises the Carousel only AFTER the media query is properly updated (a timeout is a simple enough fix, useEffect might be too complicated and interfere with the React Lifecycle). The component would then be rendered with the proper props & styling

Can compare with breakpoint 613 before and after. Before, the carousel overflows. After, the carousel doesn't overflow

Before

image

After

image

This issue took quite a while to debug

Hackin7 commented 3 months ago

To fix this rendering issue on resizing

netlify[bot] commented 3 months ago

Deploy Preview for tender-meitner-99286b ready!

Name Link
Latest commit 919196b24920f44e1b423a777d8c13ad30ff4bd7
Latest deploy log https://app.netlify.com/sites/tender-meitner-99286b/deploys/667a1902c20cfa0008db7be9
Deploy Preview https://deploy-preview-784--tender-meitner-99286b.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 90
Accessibility: 97
Best Practices: 100
SEO: 83
PWA: 70
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Hackin7 commented 3 months ago

The issue should be mostly fixed. There are some other edge cases here and there (the website needs a bit of time to update between breakpoints) but I think this is good enough to merge for the mentorship wave first

Hackin7 commented 3 months ago

There is a relatively minor bug when switching between breakpoints (or vice versa), where there is excessive padding at the bottom (but this bug is quite inconsistent also)

image

Suspected cause is that react doesn't rerender the components properly on resizing

However, I decided not to work on it as

  1. Non-breaking issue - it appears on only very specific edge cases (on typical use shouldn't be an issue), and even so very ocassionally
  2. can't really consistently recreate it + fair bit of effort to fix + I think we can spin this off into another issue.
  3. We plan to improve the carousel and tweak the design anyway, and would be better to handle this AFTER that issue