The Wayback Machine - https://web.archive.org/web/20201105053244/https://github.com/cssinjs/jss/issues/930
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

getDynamicStyles() includes Observable rules #930

Open
bhupinderbola opened this issue Dec 9, 2018 · 18 comments
Open

getDynamicStyles() includes Observable rules #930

bhupinderbola opened this issue Dec 9, 2018 · 18 comments

Comments

@bhupinderbola
Copy link

@bhupinderbola bhupinderbola commented Dec 9, 2018

Expected behavior:
Observable rules should not be included in the getDynamicStyles()

Describe the bug:
Observable rules are included in the results of getDynamicStyles()

Codesandbox link:
https://codesandbox.io/s/xrmo482nr4

Versions (please complete the following information):

  • jss: 9.8.7
  • Browser [e.g. chrome, safari]: Chrome
  • OS [e.g. Windows, macOS]: Windows
    Feel free to add any additional versions which you may think are relevant to the bug.
@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

Can you please explain why this is unexpected? Observables are a form of dynamic styles.

@bhupinderbola
Copy link
Author

@bhupinderbola bhupinderbola commented Dec 9, 2018

As per the documentation (https://cssinjs.org/js-api?v=v9.8.7#extract-dynamic-styles ),

Extracts a styles object with only props that contain function values

.
Also it creates issue with react-jss. React-jss calls sheet.update on dynamic styles. But observables do not have any data to update.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

Or we should fix the problem in react-jss by adding a check for fn values

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

Yeah, we should fix the docs and fix react-jss by adding a check.

Is there a use case for observables inside react-jss currently?

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

Same use case as with core, being able to stream the values from thee outside, for e.g. a complex animation using rx utils

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

But this would break as soon as you render two different instances of the same component.
You would need to create a new HOC for every rendered component.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

Why would it break? We don't reuse dynamic style between elements.

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

Yeah, but the observable would be the same. You can only have an animation which is shared between all components

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

ToDos for this:

  • Update docs to correctly reflect the behavior of getDynamicStyles
  • Update react-jss to filter out observables
@kof
Copy link
Member

@kof kof commented Dec 9, 2018

True that. I wonder if we should support a fn value that returns an observable.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

Alternatively separate components per animation might be also acceptable.

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

Please no. This would just create more confusion. We would also need a way to pass the observable then back to the user.

We should definitely document the problem with observables and react-jss somewhere.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

yeah, the user would have to manage observable instances.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

Probably for now user could just create separate components for each animation and scope the observable in his HOC creator function.

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

I think for most users function values should be enough.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

Totally, but then we have this Observables feature and it would be evtl nice to be able to use it with react-jss together. I don't know how to decide on whether it should be supported or not, I think if its not hard and does't causes additional bugs, we could. That thing per component can be solved in user space ... I guess your concern comes from implicitness of this behaviour one wouldn't expect?

@HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Dec 9, 2018

Yeah, exactly.

@kof
Copy link
Member

@kof kof commented Dec 9, 2018

On the other side it would be confusing not having support for observables in react-jss because it claims to support the same syntax, and Observables are part of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.