-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
[react-jss] Add proper support for observables in react-jss #1002
base: move-get-dynamic-styles
Are you sure you want to change the base?
Conversation
I opened the PR against the |
@@ -142,7 +143,8 @@ export default function withStyles<Theme: {}, S: Styles<Theme>>( | |||
...contextSheetOptions, | |||
index, | |||
meta: `${displayName}, ${isThemingEnabled ? 'Themed' : 'Unthemed'}, Static`, | |||
classNamePrefix: this.classNamePrefix | |||
classNamePrefix: this.classNamePrefix, | |||
link: getObservableStyles(themedStyles) !== null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I think because of the separation in observable and function styles, we have double overhead for those 2 function calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have one function and do it in one go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we don't need getObservableStyles, we need hasObservables() from the logic above. It could be potentially one getDynamicStyles() : {styles, hasObservables, hasFunctions}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this way it's a lot cleaner and the core doesn't need to know about observables or functions in that matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but performance is the primary priority here. I think iterating over styles is quite a bit of overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, let's evaluate how important observables are in react. I think moving the getDynamicStyles
out, still makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't evaluate that, because most people don't know this API exist. We need to promote it more, but in the end, we don't know how and were it is used.
Observables are part of the default preset, so it would be not logical if they didn't work with react-jss, which states using default preset.
Also even if we would remove observables from a default preset, it wouldn't be logical if react-jss doesn't work when user defined a new jss instance with an observable plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just need to figure out a good way to use observables with react-jss and get some use cases we can target for.
What would you like to add/fix?
Add better support for observables in react-jss.
Depends on #1000
Corresponding issue (if exists): #995