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

Styled API #1094

Merged
merged 25 commits into from Jun 12, 2019
Merged

Styled API #1094

merged 25 commits into from Jun 12, 2019

Conversation

kof
Copy link
Member

@kof kof commented Apr 23, 2019

Implements #974

Todo

  • use the new hooks based api
  • Unsupported rule rendering bug Insert a rule right after an unsupported rule will fail #1122
  • styled accepts multiple style objects or object and functions styled('div')({}, () => ({}) ...)
  • shouldForwardProp(propName) as options property styled('div', {shouldForwardProp})(style...)
  • Add emotion's label
  • Changelog

@kof kof requested a review from HenriBeck as a code owner April 23, 2019 18:46
@kof kof force-pushed the react-jss/feature/styled branch from 18c9a62 to 92fa857 Compare April 30, 2019 13:38
@kof kof requested review from HenriBeck and removed request for HenriBeck April 30, 2019 14:07
@@ -2,7 +2,7 @@

Copy link
Member Author

Choose a reason for hiding this comment

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

changes here are unrelated, but for some reason after I added tests for styled api, this tests started to fail, something weird started happening with module id, like there is a new moduleId module evaluation, I couldn't fully understand that, but I just started resetinng moduleId here.

}

// $FlowFixMe flow seems to not know that `classes` will be provided by the HOC, not by element creator.
return withStyles({css: style}, {...options, injectTheme: true})(Styled)
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is good for now, but I would like to optimize it, later on, to not render two extra react components for example.

I'm not sure if we should use the hooks implementation in here yet, as this would make the styled API only usable with react@16.8 currently, which in my opinion is a little bit early.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think performance is critical and this amount of components for each styled one is really bad, that's why I think hooks api is the only way forward with this API. For those who use older react versions they still can use the old styled implementation.

|}
}

export type Classes = {[string]: string}
Copy link
Member Author

Choose a reason for hiding this comment

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

we could export this one from the core

}

export type DynamicRules = {
[key: string]: BaseRule
}

export type ThemedStyles<Theme> = (theme: Theme) => StaticStyles
type StaticStyle = {}
Copy link
Member Author

Choose a reason for hiding this comment

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

we should use this one from the core too, for the sake of a sigle source of truth

</ThemeProvider>
</JssProvider>
)
// TODO we should not need a static rule in such cases.
Copy link
Member Author

Choose a reason for hiding this comment

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

this really needs fixing

esproposal.export_star_as=enable
suppress_comment= \\(.\\|\n\\)*\\$FlowFixMe
suppress_comment= \\(.\\|\n\\)*\\$FlowIgnore
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be used for things which need to be ignored but need no fixing

@kof kof changed the title WIP styled API Styled API Jun 12, 2019
@@ -96,7 +96,8 @@ const createUseStyles = <Theme: {}>(styles: Styles<Theme>, options?: HookOptions

useEffectOrLayoutEffect(
() => {
if (state.sheet && state.dynamicRules) {
// We only need to update the rules on a subsequent update and not in the first mount
if (state.sheet && state.dynamicRules && !isFirstMount.current) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this can be in changelog

@kof kof requested a review from HenriBeck June 12, 2019 11:56
@kof kof mentioned this pull request Jun 12, 2019
14 tasks
@kof kof merged commit 38267b1 into master Jun 12, 2019
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this pull request Sep 17, 2019
* wip styled

* more tests, lint

* add styled system tests

* switch back to style-system v3, until v4 changes the format back

* switch back to style-system v3, until v4 changes the format back

* switch back to style-system v3, until v4 changes the format back, better types

* upgrade flow and react

* upgrade theming

* flow types

* migrate to hooks

* Fix tests

* Update size snapshot

* more tests

* accept multiple static and dynamic style declarations

* implement shouldForwardProp

* merge the styles arguments passed to styled

* add test for empty values returned from function rules

* optimize perf, better types

* support label

* Update changelog.md

* skip the tests for now on the ci
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