Move Reaction's Utils/Responsive over into @artsy/responsive #1
Conversation
Typos for README.md
Got false positives?Make changes to the global settings spellcheck.json in /artsy/peril-settings. Generated by 🚫 dangerJS |
docs/Responsive.mdx
Outdated
| import { Playground, PropsTable } from 'docz' | ||
| import { Responsive } from '../src' | ||
|
|
||
| # Responsive |
| @@ -0,0 +1,5 @@ | |||
| describe("hi", () => { | |||
| it("is this working", () => { | |||
src/index.tsx
Outdated
|
|
||
| type MediaQuery = keyof typeof themeProps["mediaQueries"] | ||
|
|
||
| const { Consumer, Provider } = createResponsiveComponents<MediaQuery>() |
There was a problem hiding this comment.
@alloy - Running into the same issue as in Reaction where defining context statically makes it difficult to pass in type data. This works for Artsy's use-case, but as a general lib it would be better if we could infer these keys from props that have been passed in to the provider; e.g.,
<ResponsiveProvider <MediaQuery>
mediaQuery={...}
>
...
</ResponsiveProvider>
(I think ^ should work but need to run for now)
There was a problem hiding this comment.
This index file shouldn’t exist, instead rename Responsive.tsx to index.tsx. The idea is that the host project defines its own typed provider and consumer by using the createResponsiveComponents function. This definition of our typed version could probably move into Palette.
damassi
force-pushed
the
initial-file-addition
branch
from
f975105
to
d3460b7
Compare
README.md
Outdated
| return <div>Is xs size</div> | ||
| } else if (md) { | ||
| return <div>Is md size</div> | ||
| } else if (touch) { |
There was a problem hiding this comment.
There’s no ‘touch’ in your example media queries. Did you mean ‘hover’?
src/index.tsx
Outdated
|
|
||
| type MediaQuery = keyof typeof themeProps["mediaQueries"] | ||
|
|
||
| const { Consumer, Provider } = createResponsiveComponents<MediaQuery>() |
There was a problem hiding this comment.
This index file shouldn’t exist, instead rename Responsive.tsx to index.tsx. The idea is that the host project defines its own typed provider and consumer by using the createResponsiveComponents function. This definition of our typed version could probably move into Palette.
|
I do like this being it's on package that people can consume without pulling in the rest of our stuff... I'm not sure that having it in its own repo makes sense though. We could make palette a monorepo and have this one of the packages inside it maybe? |
|
This is def worth discussing a bit. So palette is our design system but maybe its also our framework? I’m always thinking about breaking apart Reaction into a monorepo so that things can stay fast and focused long-term, but I kinda think we should keep palette focused and Reaction the monorepo (that is, to be clear, independent of Force so that we can continue to quarentine it and eventually replace it with a slimmer, more modern service). Regarding Responsive living in its own package — I would like to hear what @alloy has to say about its overall architecture and value, but I personally think it deserves to be a stand alone lib. I would use this in all my apps since it works so darn well 😄. |
damassi
force-pushed
the
initial-file-addition
branch
from
d3460b7
to
d1f087e
Compare
|
(Moving new reaction implementation over) |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
feat: working deep nesting prevention with initial passing test
|
🚀 PR was released in |
See https://github.com/artsy/reaction/pull/1379/files
NOTE: I copied the project setup for Palette since its working well for us.