FormidableLabs / radium

A toolchain for React component styling.
http://formidable.com/open-source/radium/
MIT License
7.39k stars 308 forks source link

The Road to 1.0 #788

Open alexlande opened 8 years ago

alexlande commented 8 years ago

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 (https://github.com/facebook/react/pull/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 commented 8 years ago

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

Please pass along to anyone who might be interested :)

tptee commented 8 years ago

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 😬

robinweser commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 :) https://github.com/Khan/aphrodite/pull/104

ianobermiller commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

xcoderzach commented 8 years ago

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 commented 8 years ago

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

What do you mean by this?

xcoderzach commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

rafgraph commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

I would not like to lose this ability from Radium.

epsitec commented 8 years ago

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.

nickredmark commented 7 years ago

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

walshie4 commented 7 years ago

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

ianobermiller commented 7 years ago

Not exactly abandoned, it is still actively used by Formidable and many others. I personally don't maintain Radium anymore, maybe @alexlande can chime in?

On Mon, Mar 6, 2017 at 1:39 PM Adam Walsh notifications@github.com wrote:

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

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/FormidableLabs/radium/issues/788#issuecomment-284541602, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2ziyy5J1vaYJzP7dYRb5zdTNlxUeChks5rjH0HgaJpZM4JQHho .

isaachinman commented 5 years ago

What's the latest? I've been using Radium off and on for several years now and would like to continue using it. Is anyone actively maintaining the repo? Last commit was five months ago. I'm quite surprised this project isn't on a 1.x release after nearly three years.

I'm happy to help in any way - perhaps a collaborator or owner can give a brief update on the direction/roadmap?