feat(asCore): Convert asCore into getCoreStyles helper #376
Conversation
| @@ -9870,10 +9870,10 @@ exports[`Storyshots Config Provider Spacing Aliases 1`] = ` | |||
| style={ | |||
| Object { | |||
| "backgroundColor": "#efefef", | |||
| "borderBottomWidth": 48, | |||
There was a problem hiding this comment.
I'm stumped why these values changed. When loading storybook in browser, the styles are correct, and screenshot comparisons are passing. Is this a limitation with how context works within storyshots perhaps?
There was a problem hiding this comment.
Oh interesting it's only affecting those set by context. It does sound like an enzyme limitation not liking the changes but not sure if there's much we can do about it.
As far as we have this covered with other tests (like the image comparison ones you mentioned) I think we are good
There was a problem hiding this comment.
We discussed offline a similar approach that would allow us to turn other HOCs in the project into utility functions and retain the HOC export.
The idea is that instead of returning a JSX object they should return a plain object. Something like:
return <Component {...props} style={cssStyle} />with:
return { ...props, style: cssStyle }You can then turn a utility function back into a HOC with:
const asCore = Wrapper =>
(props, context) => <Wrapper {....myUtilAsCore(props, context)} />We may run into other limitations so not requesting changes but it seemed worth exploring.
src/asGrid/index.js
Outdated
| @@ -8,17 +8,26 @@ import type { | |||
| } from '../types' | |||
| import { styles, getCol } from './grid.styles' | |||
| import { getValue } from '../utils' | |||
| import asCore from '../core/asCore' | |||
| import { sharedResolutionProperties, getCoreStyles } from '../core' | |||
There was a problem hiding this comment.
there should be only a single default export from core. The goal is that you can either import { x } from 'gymnast or import x from 'gymnast/x' and they are both equivalent.
This allows a consumer to potentially use the babel lodash plugin to reduce the size of gymnast even further.
If these values are meant to be exported they should be available as gymnast/getCoreStyles and if not they should live elsewhere (e.g. gymnast/core/utils).
Because core is not exported, this wasn't caught by these tests but it may be useful to keep the file structure consistent
| @@ -27,8 +36,11 @@ export default function asGrid( | |||
| justify && styles[`${justify}Justify`], | |||
| ]) | |||
|
|
|||
| return <Component {...props} ref={innerRef} className={classes.join(' ')} /> | |||
| return <Component ref={innerRef} {...props} className={classes.join(' ')} /> | |||
There was a problem hiding this comment.
just curious on whether this change matters for flow or something like that
src/asGrid/index.js
Outdated
| return asCore(Grid, resolutionProperties) | ||
| return withResolution( | ||
| Grid, | ||
| sharedResolutionProperties.concat(resolutionProperties) |
There was a problem hiding this comment.
why not keep this responsibility within withResolution?
src/asLayout/index.js
Outdated
|
|
||
| return withResolution( | ||
| Layout, | ||
| sharedResolutionProperties.concat(resolutionProperties) |
src/core/index.js
Outdated
| import { combineSpacing, getValue } from '../utils' | ||
| import type { ConfigProviderContext, OneResolution } from '../types' | ||
|
|
||
| export const sharedResolutionProperties = [ |
There was a problem hiding this comment.
I wouldn't export these and just move them as part of getCoreStyles
arahansen
changed the title
|
🎉 This PR is included in version 16.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Helps reduce the depth of React tree rendering to partially fix #370. Also, some preliminary work that should make the react-native port easier.