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

useResponsiveProps: Fixes default breakpoint prop value assignment #281

Merged
merged 5 commits into from
Jan 10, 2020

Conversation

kettanaito
Copy link
Owner

@kettanaito kettanaito commented Jan 2, 2020

Changes

  • Assigns proper initial value for the state in useResponsiveProps

When rendered on a server no React hooks are run. The useEffect hook in useResponsiveProps is no exception, and this results into server-side markup containing no props at all, because the initial state of the respective useState hook is undefined. To prevent this, I make the filtering function parametric and call it with a server-side-like matcher as the initial value of the useState hook.

  • Adds a unit test for useResponsiveComponent function

GitHub

Release version

  • patch (internal improvements)
  • minor (backward-compatible changes)
  • major (breaking, backward-incompatible changes)

Contributor's checklist

@kettanaito kettanaito requested a review from ruhaise January 2, 2020 14:10
@kettanaito
Copy link
Owner Author

Hi, @ruhaise. Could you please take a look at this pull request?

I've tried to address the issue you were fixing in #276 using a custom matching function during responsive props filtering. Tests pass, but I've found out that we cannot currently handle the "down" behavior, if it affects the default breakpoint also (see the skipped test for useResponsiveComponent.test.ts). I'm still thinking how to resolve that properly.

@kettanaito kettanaito force-pushed the 275-ssr-use-responsive-props branch from 081f909 to d1797ab Compare January 2, 2020 14:13
@kettanaito kettanaito requested a review from ruhaise January 2, 2020 14:32
ruhaise
ruhaise previously approved these changes Jan 2, 2020
@ruhaise ruhaise requested review from ruhaise and removed request for ruhaise January 3, 2020 13:17
Copy link
Collaborator

@ruhaise ruhaise left a comment

Choose a reason for hiding this comment

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

i had no clue how to remove the approval state

@ruhaise ruhaise requested review from ruhaise and removed request for ruhaise January 3, 2020 13:19
@kettanaito kettanaito requested a review from ruhaise January 4, 2020 21:33
@kettanaito
Copy link
Owner Author

@ruhaise, I'm not sure what's the issue. I've requested another review from you, so feel free to approve or request changes with commenting on places you think can be improved. Thanks!

@kettanaito
Copy link
Owner Author

I think I'll file a separate issue regarding how the "down" behavior is handled. The thing is that this issue is reproducible even in the latest release, so it can be tackled separately if its complexity is too high for this pull request. I'd rather spend some time thinking how to approach this.

@ruhaise
Copy link
Collaborator

ruhaise commented Jan 5, 2020

@kettanaito approved and added a very minor comment

@kettanaito kettanaito force-pushed the 275-ssr-use-responsive-props branch from 2c20c27 to c5754e9 Compare January 8, 2020 13:21
@kettanaito kettanaito merged commit cd790db into master Jan 10, 2020
@kettanaito kettanaito deleted the 275-ssr-use-responsive-props branch January 10, 2020 08:50
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.

useResponsiveComponent prop values not applied during SSR
2 participants