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

The Road to 1.0 #788

Open
alexlande opened this Issue Jul 19, 2016 · 29 comments

Comments

Projects
None yet
@alexlande
Copy link
Member

alexlande commented Jul 19, 2016

Development on Radium has stalled lately. The timing of this is unfortunate because there are several large bugs in the project that need to be dealt with soon. I have some thoughts about what our next steps should be to get those resolved and make Radium more robust going forward.

The Problems

There are two primary issues with Radium at the moment, in my opinion.

Vendor Prefixing/Fallback Values

React 15 broke the (admittedly hacky) behavior we were relying on to provide vendor prefixes and fallback values in inline styles. This is very bad. For discussion of this issue, see #602. The React team is aware of the issue and there is a WIP PR to resolve it (facebook/react#6701), but it's not clear if it's going to land and I don't think we should count on it.

Pseudo-Classes Are Hard

Right now, we emulate the CSS pseudo-classes :hover, :focus, and :active. Unfortunately for us, there are a whole slew of bugs (both now and in the past) related to this behavior, because there are a lot of edge cases around these behaviors.

The Solution

Both of these issues would be resolved by generating CSS in a style tag using the addCSS behavior that we use for media queries. For discussion of this idea, see #510. Moving in this direction (as Aphrodite has done, as a good example) would mean that all of the issues with both inline styles themselves (like fallback values) and emulating browser behavior would go away. It would also mean that any pseudo-elements/classes/selectors would work. This would remove a lot of behavior that Radium emulates now, and would be extremely flexible.

This would be a breaking change as some APIs, like getState, would go away completely. Other than that, my hope is that Radium would largely work as it does now, as the API is very good. The only real change would be the way that styles are applied.

Known Issues

One thing that is likely to hold us back here is the ongoing issue with media query class names and unique-ness. See #635. This is something that we would need to handle (and that we need to deal with as soon as possible anyway, because it's a major bug in its own right) before we could ship with even more generated CSS.

Thoughts?

I would love to hear any thoughts on this idea. I believe it's the right way to go, but want to hear from the community.

@alexlande

This comment has been minimized.

Copy link
Member

alexlande commented Jul 19, 2016

/cc @ianobermiller @tptee @coopy @doctyper @philholden @rofrischmann

Please pass along to anyone who might be interested :)

@tptee

This comment has been minimized.

Copy link
Contributor

tptee commented Jul 19, 2016

After trying a mess of stuff to get #635 resolved, I think the simplest solution is to just order the media queries. Not only does it simplify the deduplication story, it also restores the media query behavior that designers understand and depend on. Yes, it means that rules inside media queries aren't necessarily precedence-order. I think it's worth it.

Obviously a breaking change 😬

@rofrischmann

This comment has been minimized.

Copy link
Contributor

rofrischmann commented Jul 20, 2016

As I earlier mentioned I totally believe that transforming to CSS is the correct way to go 👍 Also using the prefixer is much simpler when transforming as you can use all fallbacks again. You might also want to check out how I handled specificity, order and naming issues within Fela as media query order is not as trivial as is might sound.

I think the getState API is really helpful for special cases, but 98% do not use it (I guess). You might provide it separately e.g. using another HoC, but it should not be in Radium by default.
According the other issues: Using CSS simply solves those issues right? You can do pseudo classes, fallbacks, prefixes and even exotic stuff like :hover > #id.class .inner-class h1 (though it is a terrible idea of course). Also you do not need to add any !important again, which I believe is really hacky (I am not sure if Radium uses !important at all, but saw that aphrodite is using those, which again allows users to overwrite styles from outside the component (which can be good and bad as well).

@doctyper

This comment has been minimized.

Copy link
Contributor

doctyper commented Jul 20, 2016

Having considered a move to Aphrodite (at one point I considered writing a custom Radium plugin that transformed all inline styles into CSS), I am definitely on board with moving Radium closer to their implementation. The NFL team likes Radium, but I have heard some negative feedback, particularly around media query resolution, hover states, getState and overall state management.

👍 transforming to CSS.

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 21, 2016

Other than that, my hope is that Radium would largely work as it does now, as the API is very good. The only real change would be the way that styles are applied.

@alexlande This is the most important part. If we don't believe the API Radium provides is worthwhile, then there is no point in trying to keep this library around while pulling the good parts out of other libraries. I agree with you, the API Radium provides is good and worthwhile, and that this is the right way to preserve that experience while fixing the longstanding issues.

You might also want to check out how I handled specificity, order and naming issues within Fela as media query order is not as trivial as is might sound.

@rofrischmann Can you point us to the specific code? Does Aphrodite handle this correctly?

Also you do not need to add any !important again, which I believe is really hacky (I am not sure if Radium uses !important at all, but saw that aphrodite is using those, which again allows users to overwrite styles from outside the component (which can be good and bad as well).

@rofrischmann Aphrodite just landed a PR for an optional no-important mode :) Khan/aphrodite#104

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 21, 2016

overall state management

@doctyper Can you expand on this one? I think the other things we already have issues open for, but I've not heard general complaints about "state management".

@alexlande

This comment has been minimized.

Copy link
Member

alexlande commented Jul 21, 2016

Thanks for the feedback all, I'm excited to get this going. I'll open a new ticket to look at the media query order/specificity issue, since that will be something we need to take care of early on.

Edit: Opened, see #790.

@doctyper

This comment has been minimized.

Copy link
Contributor

doctyper commented Jul 21, 2016

@ianobermiller The main complaint I've heard is that you have to be careful when using hover states or media queries in Radium because it may cause performance issues in your component, depending on your use. It's mitigated by using a proper shouldComponentRender strategy, but it's an implementation detail that's not required when using straight CSS.

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 21, 2016

@doctyper Thanks, that makes sense. That would certainly be fixed by a new approach.

@xcoderzach

This comment has been minimized.

Copy link

xcoderzach commented Jul 21, 2016

How will dynamic styles be handled? If you do something like

<div style={dynamicStyle(this.state.calculatedWidth)} />

You don't want to pay the "Recalculate Style" on every frame.

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 21, 2016

You don't want to pay the "Recalculate Style" on every frame

What do you mean by this?

@xcoderzach

This comment has been minimized.

Copy link

xcoderzach commented Jul 21, 2016

When you append a style to a stylesheet, chrome devtools puts a "Recalculate Style" in the timeline, which usually takes significantly longer than just inlining the style.

I ran into this when building an animation library. I tried to build styles, and then insert them into a sheet and then apply a class, and there was a huge (100-200ms on mobile) slowdown. Due to "Recalculating Styles"

If I remember correctly, when you append a style to a stylesheet, every style in the sheet is recalculated.

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 21, 2016

Ah, so your point is basically that you can't use the style prop to do inline styles anymore, which are basically required for animation. I agree that it would be weird to transform the style prop to 100% CSS. If you switch to className, you get something similar to Aphrodite, if you switch to look you get react-look ;).

@xcoderzach

This comment has been minimized.

Copy link

xcoderzach commented Jul 21, 2016

@ianobermiller Yeah, I'm pretty sure Aphrodite gets away with it by eagerly generating and appending to the sheet as soon as Stylesheet.create is called.

But it doesn't just affect animations, you'd need to precompute all possible dynamic styles. A common thing I like do is:

<div style={buttonStyle("active")}>

Which wouldn't work if you compute it up front, and would cause render jank if you dont.

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 21, 2016

Unless your animating there shouldn't be noticeable jank. Aphrodite does not eagerly append, the StyleSheet.create code does very little. From the readme:

Injects only the exact styles needed for the render into the DOM

@xcoderzach

This comment has been minimized.

Copy link

xcoderzach commented Jul 21, 2016

Oh, cool, then this definitely sounds like the right move. And if you're going for high performance animations, you probably shouldn't use radium on the component anyways.

@xcoderzach

This comment has been minimized.

Copy link

xcoderzach commented Jul 21, 2016

Given the current radium api, how would you generate a class, given a style object? Some sort of object hashing function? Or would the class change on every render?

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 22, 2016

Definitely would hash the contents, we would NOT want to update the stylesheet on render if the styles haven't changed.

@rafrex

This comment has been minimized.

Copy link

rafrex commented Jul 28, 2016

Sticky hover bug on touch devices (both Radium and CSS)

Currently with Radium the :hover state sticks after tapping an element on a touch device, and doesn't revert to normal until tapping someplace else (also, the :active state doesn't appear when your finger is on an element). This is because the mouseenter event doesn't fired until after the touchend event, and the mouseleave event doesn't fire until you tap someplace else on the screen.

I know you're planing on moving to use CSS for :hover, but that also suffers from the sticky hover bug as well (it was a "feature" to accommodate hover menus in the early days of mobile, but it is now a bug IMO). I came up with a fix for the CSS sticky hover bug which allows for 3 unique states: :hover, :active: and touch :active, but it's not perfect as some mobile browsers are a little buggy when working with the CSS :active state, especially when there are a lot of links on the page (the links still work fine, but the browser won't show the :active style on touch sometimes).

FWIW I think a js/react solution that provides 3 unique states, hover, active, and touchActive would work better. Or if you go the CSS route, then incorporate something like current-input so that using Radium will automatically fix the sticky hover bug (and provide a unique touchActive state that can be styled separately).

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Jul 30, 2016

FWIW I think a js/react solution that provides 3 unique states, hover, active, and touchActive would work better

@rafrex I suggest opening a separate issue proposing just that :)

@philholden

This comment has been minimized.

Copy link

philholden commented Aug 10, 2016

My main reason for using Radium is that I want fine control over styles at runtime and style isolation. I use Radium to update (x, y) translate positions for drag and drop and swipe gestures on mobile. I think it is important that these use cases remain as fast as possible that said I have not investigated the current overhead of using Radium vs plain style.

@philholden

This comment has been minimized.

Copy link

philholden commented Aug 15, 2016

Just had a play with Aphrodite. It seems nice. Where it falls down for me is that if you update styles in your render e.g. randomize a color you get a new style class appended on every render and they accumulate:

<style type="text/css" data-aphrodite="">
.red_1edo4uo{color:#899f6e !important;}
.red_pgvrla{color:#b60c44 !important;}
.red_enqibb{color:#8ac16f !important;}
.red_1l6nd5j{color:#4deb72 !important;}
.red_1hk3c3e{color:#4e40e3 !important;}
.red_1yudl4g{color:#301b18 !important;}
.red_1266jhv{color:#e56929 !important;}
</style>

I want to be able to have theme colors I can adjust with sliders. I could not find a way to get Aphrodite to clean up after itself.

@doctyper

This comment has been minimized.

Copy link
Contributor

doctyper commented Aug 15, 2016

Hm. It makes sense does it not? I'm sure they are generating that hash based on the style values, which would re-gen that rule.

@philholden

This comment has been minimized.

Copy link

philholden commented Aug 16, 2016

Yes the id base on hash is good because you do not want 1000 classes defined when you have a table. But it would be nice if there was a way to remove CSS that was no longer needed. e.g. have something you could put in componentWillUnmount which knows when the last element that uses a CSS class has been removed. This is hard to do in Aphrodite because it targets non React apps as well. This would make me classify Aphrodite as compile time styles because you cannot change individual style props in response to component props without leaving behind a trail of unwanted css classes.

If Radium wants to differentiate itself from Aphrodite it needs to keep offering runtime styles. Which I think is just a case of having a HoC that keeps track of which CSS classes are being used and removes unused CSS classes on componentWillUnmount.

(need to remove unused classes on render too)

@philholden

This comment has been minimized.

Copy link

philholden commented Sep 15, 2016

Added playlist on React Recompose: Theme UI Libraries at Runtime Using Context

I would not like to lose this ability from Radium.

@epsitec

This comment has been minimized.

Copy link
Contributor

epsitec commented Sep 15, 2016

Our code relies on getState in order to update some child element when the parent gets hovered.
If you remove getState in Radium (because it does not make sense for most users), I'd be really glad to see that functionality provided as a separate package.

@nmaro

This comment has been minimized.

Copy link

nmaro commented Feb 21, 2017

Any news on this? Is this project abandoned (last commit 5 months ago...)? Anyone found a good alternative?

@walshie4

This comment has been minimized.

Copy link

walshie4 commented Mar 6, 2017

@nmaro https://github.com/cssinjs/jss has a similar feature set (albeit different approach) but achieves a lot of the same functionality.

@ianobermiller

This comment has been minimized.

Copy link
Collaborator

ianobermiller commented Mar 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment