Skip to content

Conversation

@jonkafton
Copy link
Contributor

What are the relevant tickets?

Fixes https://github.com/mitodl/hq/issues/5471

Description (What does it do?)

Replaces all uses of useMuiBreakpointAtLeast() with css-only media queries. The reason is that the hook uses window.matchMedia(), which is not availalbe in the server for server rendered content.

Also fixes css issues with the headings in the homepage Testimonials and News / Events that I noticed (present in RC and Prod).

Whitelists all image URLs in the config.

Screenshots (if appropriate):

image

How can this be tested?

Load the homepage.

  • The News and Events section should appear and behave as on prod at all screen widths.

Load the search page

  • The search results should display the mobile loading state as smaller screen widths and the desktop loading state at larger screen widths

Additional Context

@jonkafton jonkafton changed the title Migrate Migrate useMediaQuery hooks Sep 16, 2024
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Sep 16, 2024
@shanbady shanbady self-assigned this Sep 17, 2024
@shanbady shanbady self-requested a review September 17, 2024 13:36
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

Could you undo this change to Testimonials page? To avoid conflicts with #1568

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

I also see two usages of useMuiBreakpointAtLeast remaining:

  • frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCardCondensed.tsx
  • frontends/ol-components/src/hooks/useBreakpoint.ts

Additionally, it might be good to add

          {
            name: "@mui/material/useMediaQuery",
            message: "Use CSS instead; with SSR, useMediaQuery can cause an initially different styling. ",
          },

to the @typescript-eslint/no-restricted-imports rule in eslintrc

@jonkafton
Copy link
Contributor Author

jonkafton commented Sep 18, 2024

I also see two usages of useMuiBreakpointAtLeast remaining:

Urrg sorry, this branch was missing a cherry pick after rebase / cleanup. Let's revisit our approach of rebasing main onto the nextjs. I'm increasingly thinking its not viable.

@jonkafton jonkafton force-pushed the jk/5471-migrate-media-queries branch from c2a1c54 to 1e6be01 Compare September 20, 2024 19:53
@jonkafton jonkafton removed the Needs Review An open Pull Request that is ready for review label Sep 23, 2024
@jonkafton jonkafton merged commit f2124ed into nextjs Sep 23, 2024
@jonkafton jonkafton deleted the jk/5471-migrate-media-queries branch September 23, 2024 19:49
jonkafton added a commit that referenced this pull request Sep 23, 2024
* Whitelist all image URLs

* Replace media query hook with CSS only

* Update news and events

* Fixes homepage text styles

* Breakpoint at md

* Update condensed resource card. Remove use breakpoint.

* Update tests for homepage desktop and mobile headings both present
jonkafton added a commit that referenced this pull request Sep 23, 2024
* Whitelist all image URLs

* Replace media query hook with CSS only

* Update news and events

* Fixes homepage text styles

* Breakpoint at md

* Update condensed resource card. Remove use breakpoint.

* Update tests for homepage desktop and mobile headings both present
jonkafton added a commit that referenced this pull request Sep 24, 2024
* Whitelist all image URLs

* Replace media query hook with CSS only

* Update news and events

* Fixes homepage text styles

* Breakpoint at md

* Update condensed resource card. Remove use breakpoint.

* Update tests for homepage desktop and mobile headings both present
@odlbot odlbot mentioned this pull request Oct 22, 2024
74 tasks
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.

4 participants