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

Replace makeResponsive with useMediaQuery #3032

Open
taneliang opened this issue Dec 7, 2020 · 6 comments
Open

Replace makeResponsive with useMediaQuery #3032

taneliang opened this issue Dec 7, 2020 · 6 comments

Comments

@taneliang
Copy link
Member

taneliang commented Dec 7, 2020

useMediaQuery was added in #3007 to replace the makeResponsive higher order component. This issue tracks the removal of the deprecated makeResponsive from the codebase.

At the time of writing, there are 5 remaining components that use makeResponsive, 2 of which are function components. The remaining 3 class components will need to be replaced with function components first (similar to work done in #3007 and #3009).

This can be done over multiple PRs depending on your comfort level.

Update: only TodayContainer is left. VenuesContainer too, but that'll be done in #3038.

@taneliang taneliang changed the title Replace makeBreakpoint with useMediaQuery Replace makeResponsive with useMediaQuery Dec 7, 2020
@BransonNg
Copy link
Contributor

Hi! Can i take a work on this :)

@taneliang
Copy link
Member Author

@BransonNg sure, thank you! Just leave out VenuesContainer for now as I'm reworking it.

Let me know if you have any questions!

@BransonNg
Copy link
Contributor

Hi @taneliang, was going through TodayContainer.test.tsx, and am having trouble testing the useEffect cycle as I'm unable to mount or render TodayContainer due to some Link components being rendered outside of a Router component.

Any suggestions as to how to test this?

Also related to the same test suite, any suggestions for checking the weather state? (i'm unaware of a way to access a component's useState to assert any tests)

@taneliang
Copy link
Member Author

Hey @BransonNg!

Sorry for the delay in replies, I'm having In-Camp Training these 2 weeks.

Would renderWithRouterMatch fix the Link stuff? https://github.com/nusmodifications/nusmods/blob/master/website/src/test-utils/renderWithRouterMatch.tsx Alternatively we could also manually wrap the TodayContainer in a MemoryRouter like we do with some other tests, since we don't actually need a route match.

Regarding the weather test, off the top of my head (I haven't looked at the test or the code yet) maybe we could try one of these approaches:

  1. Use https://mswjs.io/ to mock the weather service. This approach is recommended by RTL's author.
  2. Mock the Axios/fetch request for weather data (assuming we're using one of them). This would be a lot easier but there's a good chance that we'll want to dump Axios soon in favour of SWR or React Query, so I'd slightly prefer the MSW approach if it's not too complicated.

We can then just assert on the UI contents with a find* matcher.

@BransonNg
Copy link
Contributor

oh okay, i will look into that. Thanks @taneliang!

Also, would it be possible for me to add this dependency?
https://github.com/immerjs/use-immer
Would like to use it in the FC in VenuesContainer to manage the state

@taneliang
Copy link
Member Author

Oh, you can leave out VenuesContainer as I've already done it in #3038 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants