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

Broadcast React theme changes through shouldComponentUpdate barriers #239

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Broadcast React theme changes through shouldComponentUpdate barriers #239

merged 2 commits into from
Apr 25, 2017

Conversation

benoneal
Copy link
Contributor

@benoneal benoneal commented Apr 6, 2017

No description provided.

@robinweser
Copy link
Owner

Thank you! I will check it as soon as possible, but I'll be away for 1,5 weeks sadly. Can you quickly answer these questions, then I might release it today!

Does it change any behavior (is there any breaking change)?
How does it affect the package size?

Greets, Robin :)

@benoneal
Copy link
Contributor Author

benoneal commented Apr 7, 2017

@rofrischmann No breaking changes. I've confirmed this by swapping it in to my project. Only difference is the bug is fixed :)

Package size would be increased by the size of the react-broadcast lib, which is pretty tiny.

@robinweser robinweser merged commit 29af084 into robinweser:master Apr 25, 2017
@veeramarni
Copy link

Is this migrated to master?

@namjul
Copy link
Contributor

namjul commented Sep 4, 2017

It seams this has been removed. Is there a way this can be reimplemented?
How Gamorous implemented it paypal/glamorous#24

@robinweser
Copy link
Owner

Definitely an idea we should invest some time again. It was removed as it did not cover all cases and therefore didn't work properly.

@namjul
Copy link
Contributor

namjul commented Sep 4, 2017

What about implementing it the way glamorous or styled-components did (they seam to have the same approach)?

@namjul
Copy link
Contributor

namjul commented Sep 5, 2017

For now i wrote my own ThemeProvider with a custom createComponent which wraps fela's createComponent and uses an custom withTheme hoc to provide the theme as a prop.

You override the theme prop in https://github.com/rofrischmann/fela/blob/master/packages/fela/src/bindings/createComponentFactory.js#L73.
Is it possible to change it to ruleProps.theme = theme || ruleProps.theme || {}
So i can use the prop theme instead of something else.
I would provide the PR.

@robinweser
Copy link
Owner

Would you mind sharing those changes? I was about to implement theming but sadly they're still not available for preact/inferno abstractions.

@namjul
Copy link
Contributor

namjul commented Sep 5, 2017

I basically recreated it using the approach from glamurous

And my custom createComponent looks like

// createComponent.js
import { PropTypes, createElement } from 'react'
import { createComponentFactory } from 'fela'
import withTheme from 'hocs/withTheme'

const componentFactory = createComponentFactory(createElement, {
    renderer: PropTypes.object,
})

export default function (
    rule,
    type,
    passThroughProps
) {
    return withTheme(componentFactory(rule, type, passThroughProps))
}

@benoneal
Copy link
Contributor Author

benoneal commented Sep 6, 2017

If anyone wants a working solution now, I created Freyja, which is a convenience layer around Fela, and solves my three biggest issues with Fela:

  • Themeing through shouldComponentUpdate
  • Vendor prefixing correctly for ALL styles in IE10+ (the lib Fela uses has gaps and is un-optimized)
  • Eliminating pointless boilerplate, such as passing around the renderer and exposing implementation details to components

It's incredibly simple to use and has my favourite API: styles are a single function that returns an object, and the keys of those that object become your className accessors:

import withStyles from 'freyja'

const styles = ({ theme, ...props }) => ({
  wrapper: { backgroundColor: theme.colors.red }, 
  title: { ...theme.typography.h1 }
})

const Title = ({ title, styles }) => 
  <div className={styles.wrapper}>
    <h1 className={styles.title}>{title}</h1>
  </div>

export default withStyles(styles)(Title)

And as an added bonus it has built in memoization of rule rendering, and comes with some optional helper methods, such as fluidType and pseudo-element styling and a few others, which I often add to my theme object to make styling easier.

@TxHawks
Copy link
Collaborator

TxHawks commented Sep 6, 2017

@benoneal - one downside to using the API you outlined above is that it wont enjoy the optimizations afforded by babel-plugin-fela as it only optimizes components created using the createElement HoC

@benoneal
Copy link
Contributor Author

benoneal commented Sep 6, 2017

@TxHawks Sure. Though Freyja does memoize the rendering of styles per key, which optimizes even for dynamic styles.

In my applications I rarely, if ever, have purely static styles, perhaps excepting the most trivial components. I make highly interactive and runtime cusomizable interfaces so almost everything can change based on props and dynamic themes.

I imagine if your use-case were heavily dominated by static styles and had a lot of hot rendering paths, the optimization provided by that plugin might beat out raw memoization enough that it was worth the effort of wrapping everything in the createElement HoC and incorporating that plugin into the build-system.

@namjul
Copy link
Contributor

namjul commented Sep 11, 2017

@benoneal thx i will use freyja a an inspiration (we don't need the extra vendor prefixing)
@rofrischmann when do you think there will be theme support (as mentioned) in fela?

@namjul
Copy link
Contributor

namjul commented Sep 11, 2017

@rofrischmann nevermind didn't saw #302

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.

5 participants