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

Adds static media query matcher for SSR scenarios #288

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kettanaito
Copy link
Owner

@kettanaito kettanaito commented Jan 12, 2020

Changes

  • First, isolates the part of a logic of createMediaQuery that's responsible for parsing a Breakpoint into a list of query params. Adds createQueryList function.
  • Changes the end result of normalizeQuery to return a { prefix, name, value }, which is much more flexible data structure to work with, and also a backward-compatible one.
  • Adjusts useMediaQuery hook for styled-components implementation to use the updated call signature of joinQueryList

GitHub

Release version

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

Contributor's checklist

  • My branch is up-to-date with the latest master
  • I ran yarn verify and verified the build and tests passing

@kettanaito kettanaito force-pushed the 284-ssr-match-media branch 3 times, most recently from 85c57d2 to 023ac58 Compare January 13, 2020 08:45
@kettanaito kettanaito changed the title Adds static media matcher for SSR scenarios Adds static media query matcher for SSR scenarios Jan 13, 2020
@kettanaito
Copy link
Owner Author

kettanaito commented Jan 13, 2020

Knowing all responsive props declarations of a single prop

I need to keep in mind that simply introducing a static alternative for matchMedia to run on Node environment is not enough. The entire difficulty of the issue is that the logic (in useResponsiveProps) needs to know if a current responsive prop declaration has any other declarations with different breakpoints/behaviors that may affect the decision to include that prop during the SSR.

There is definitely such responsive props grouping done for areas/template, and there's a fair chance the logic may be grouping all props that way. That would be neat, as it would allow to operate with responsive prop using an interface like { [propName: string]: Declarations[] }.

Caveat 1: When the responsive props should be parsed so that seemingly any part of logic could access the parsing result? It'd be inefficient to parse at each application point.

@kettanaito
Copy link
Owner Author

kettanaito commented Jan 13, 2020

Breakpoint/client dimensions comparison with mixed Numeric values

Current implementation of the library respects Layout.defaultUnit when delcaring dimensions for Layout.breakpoints:

Layout.configure({
  defaultUnit: 'rem',
  breakpoints: {
    mobile: {
      maxWidth: 15 // becomes "15rem",
      minHeight: "500px" // stays explicit 500px
    }
  }
})

With this it's possible to have one breakpoint defined in numers (rems in the example above), and one in explicit measurement (i.e. "500px"—pixels).

During the SSR a static media query matcher function would have to compare the actual (default breakpoint assumed on the server) and the expected (responsive prop's breakpoint, i.e. titleMd) breakpoints to match.

How would one compare?

15 ??? '500px'

Since the value of "rem" is unknown on the server it becomes an unknown variable in this comparison.

Potential solution

To solve this, Atomic Layout may bring a rule that Layout.breakpoints dimensions are always written using Layout.defaultUnit. Meaning, you cannot use stings for explicit value, like providing a dimension in pixels if your default unit is rems. This way comparison would be conducted in the same unit and it's unnecessary to know the value of a rem to compare its incremental (the same unknown on both sides of the comparison can be omitted).

@kettanaito kettanaito added the needs:discussion Further information is requested label Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:discussion Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useResponsiveProps does not respect behavior on SSR
1 participant