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

Use own "MediaQuery" component over "react-responsive" #115

Merged
merged 8 commits into from
Mar 7, 2019

Conversation

kettanaito
Copy link
Owner

@kettanaito kettanaito commented Feb 16, 2019

These changes aim to provide a custom implementation of MediaQuery component to use in Atomic layout package.

Bundle size reduction is currently ~6 Kb (unminified).

Motivation

At the moment Atomic layout relies on a single dependency react-responsive. It composes nearly a half of the atomic-layout package size, and has a deep dependency tree of its own. Everything gets bundled and shipped under atomic-layout package.

Dependencies investigation has shown that some of the deep dependencies of react-responsive are obsolete, and contain implementation that causes unexpected behaviors (found out during SSR exceptions due to invalid media query RegExp). Those directly affect the behavior of Atomic layout.

Goals

  • Decrease the amount of dependencies and sub-dependencies
  • Decrease the bundle size shipped
  • Decrease the amount of bug caused by deep out-of-date dependencies (Tests for SSR #72)
  • Decrease the amount of bundled unused code from external libraries (tree shaking doesn't help)
  • Increase control over the responsive media queries

Roadmap

  • Remove react-responsive dependency and its usage
  • Introduce a basic MediaQuery component
  • Improve media query from props composition in MediaQuery components
  • Ensure unit tests pass
  • Ensure integration tests pass
  • Ensure smooth behavior on runtime (manual testing)
  • Restore test coverage
  • Conduct testing with SSR and report the server-side and client-side hydration behavior

@coveralls
Copy link

coveralls commented Feb 17, 2019

Coverage Status

Coverage increased (+0.5%) to 90.436% when pulling d8c31eb on media-query-replacement into b6713b6 on master.

@kettanaito
Copy link
Owner Author

kettanaito commented Mar 5, 2019

Performing manual testing... Will write report once done.


Report

Own MediaQuery component substitutes the previous one to the full extent. However, these has been the following issues found during manual testing:

  1. Children rendered by MediaQuery "jump in" on the client side, causing bumps in the UI. This is caused by the initial state of the matches property in the component. Once the matchMedia evaluation is provided instead of it, the bumps issue is gone:

const [matches, setMatches] = React.useState(
/* (?) Use some "staticMatch" with explicit values */
typeof matchMedia !== 'undefined' ? matchMedia(query).matches : false,
)

  1. Using matchMedia breaks SSR (there is no matchMedia on the server).

@kettanaito
Copy link
Owner Author

The usage of matchMedia can be omitted on the server, making a fallback to static false. This is not ideal, as it will produce nodes mismatch between server rendered content and client side. However, this is a much bigger conceptual question, which I cannot solve as a part of this pull request.

@kettanaito kettanaito changed the title Uses custom "MediaQuery" component over "react-responsive" Use own "MediaQuery" component over "react-responsive" Mar 6, 2019
@kettanaito
Copy link
Owner Author

kettanaito commented Mar 6, 2019

Automated and manual testing passes. The only thing left is to restore/improve test coverage.

Update: Test coverage should be restored now, but Coveralls is stuck reporting.

@kettanaito kettanaito merged commit da009b5 into master Mar 7, 2019
@kettanaito kettanaito deleted the media-query-replacement branch March 7, 2019 13:38
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.

None yet

2 participants