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

[react-jss] Add proper support for observables in react-jss #1002

Open
wants to merge 1 commit into
base: move-get-dynamic-styles
Choose a base branch
from

Conversation

HenriBeck
Copy link
Member

What would you like to add/fix?
Add better support for observables in react-jss.

Depends on #1000

Corresponding issue (if exists): #995

@HenriBeck HenriBeck requested a review from kof January 27, 2019 12:56
@HenriBeck
Copy link
Member Author

I opened the PR against the move-get-dynamic-styles branch, so the diff is not cluttered.

@@ -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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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}

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 think this way it's a lot cleaner and the core doesn't need to know about observables or functions in that matter.

Copy link
Member

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.

Copy link
Member Author

@HenriBeck HenriBeck Jan 27, 2019

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.

Copy link
Member

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.

Copy link
Member

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.

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.

2 participants