cloudflare / cf-ui

:gem: Cloudflare UI Framework
Other
1.29k stars 81 forks source link

Stop passing rule functions downstream #278

Closed tajo closed 7 years ago

tajo commented 7 years ago

This prevents to append custom classNames to components styled by Fela.

It gives a warning:

You cannot restyle Fela component ${componentName}
koddsson commented 7 years ago

Nice conventional commit 👌 🎉

wyuenho commented 7 years ago

So I see you've basically just inlined createComponentFactory from here, with a few minor changes.

I admire this attempt to solve this problem, but this approach falls far shorter than what's necessary to do what you want to achieve.

I can still do this:

import { createComponent } from 'react-fela';
import { Button } from 'cf-component-button';
const MyButton = createComponent(() => ({background: 'black'}), Button, Object.keys(Button.propTypes));

This is still not hard at all to do at all, and easy to miss when reviewing PRs.

An additional problem with this approach is, fela doesn't have to tell us when they violate semver and break the internal machinery of fela-utils, I'm very hesitant to use something fela explicitly said you should only use for fela internals.

What you can possibly do is to monkey-patch fela in our application, but I'm not sure that will pass code review.

tajo commented 7 years ago

@wyuenho

So I see you've basically just inlined createComponentFactory from here, with a few minor changes.

Yep. I made it simpler.

I don't think that your example really works and even if it did, react-fela will soon not be a dependency of any of our projects. Also, I am more than happy to enforce that through eslint rule (together with not importing cf-style-const and other fela packages) if people try to add that.

fela doesn't have to tell us when they violate semver

It will.

tajo commented 7 years ago

@koddsson It can't be pushed upstream because Fela's implementation is more permissive and cover more cases which makes sense (for example it needs to play nicely with legacy CSS and classNames or support for React Native).

wyuenho commented 7 years ago

Okay, banning importing from fela via an eslint rule will make it sufficiently harder for me to override styles.

wyuenho commented 7 years ago

I don't think that your example really works and even if it did, react-fela will soon not be a dependency of any of our projects.

Are we just going to wrap everything react-fela exports? That sounds good to me too.

tajo commented 7 years ago

Are we just going to wrap everything react-fela exports? That sounds good to me too.

Yes. We already do it. I just needed to update <StyleProvider> (merged today) so it accepts outside renderer and works in next.

tajo commented 7 years ago

Added another commit that fixes propType warnings. It passes propTypes to the outer component, so it works and uses component name for displayName instead of style function name (which is often anonymous or not useful anyway).

<Button />

gives

screen shot 2017-06-27 at 10 32 35 am