Feature: theme listener #3

iamstarkov · 2017-05-21T17:41:55Z

as a way to create custom High-Order Components which are supposed to react to or interact with theme updates.

as an example withTheme has been refactored and became very lightweight and simple:

class withTheme extends React.Component {
  static contextTypes = themeListener.contextTypes;
  constructor(props) {
    super(props);
    this.state = { theme: {} };
    this.setTheme = theme => this.setState({ theme });

    this.themeListenerInit = themeListener.init.bind(this);
    this.themeListenerSubscribe = themeListener.subscribe.bind(this);
    this.themeListenerUnsubscribe = themeListener.unsubscribe.bind(this);
  }
  componentWillMount() {
    this.themeListenerInit(this.setTheme);
  }
  componentDidMount() {
    this.themeListenerSubscribe(this.setTheme);
  }
  componentWillUnmount() {
    this.themeListenerUnsubscribe();
  }
  render() {
    const { theme } = this.state;
    return <Component theme={theme} {...this.props} />;
  }
};

coveralls · 2017-05-21T17:48:06Z

Coverage decreased (-0.3%) to 94.068% when pulling aa1cb576b7bfe084fc5e8964f1ca38a4b05dc94f on feat/theme-listener into af0faaa on dev-review.

iamstarkov · 2017-05-21T17:51:15Z

If themeListener offers good enough level of abstraction, then i'll write tests and docs for it. then lets merge it

iamstarkov · 2017-05-21T18:00:30Z

If themeListener offers good enough level of abstraction

if it doesnt, lets iterate and think of smth

kentcdodds · 2017-05-21T19:06:23Z

Honestly, I'm not the right person to ask about the theme functionality in glamorous. I neither developed it nor use it. @vesparny was the one who implemented it, so he probably has some thoughts :)

mxstbr · 2017-05-22T07:22:26Z

I like this, seems like a good level of abstraction!

iamstarkov · 2017-05-22T08:00:36Z

@mxstbr nice

iamstarkov · 2017-05-22T08:00:57Z

@kof your turn

kof · 2017-05-22T08:20:59Z

src/create-theme-listener.js

+const PropTypes = require('prop-types');
+const channel = require('./channel');
+
+function createThemeListener(CHANNEL = channel) {


I would keep the channel lower case, because this mapping from lower to upper doesn't makes sense or look good. Knowing that its a constant doesn't give you any benefits when using in this way.

its minor implementation detail, and lets discuss to change it later in separate issue

kof · 2017-05-22T08:23:20Z

src/create-with-theme.js

+      static contextTypes = themeListener.contextTypes;
+      constructor(props) {
+        super(props);
+        this.themeSubscribe = themeListener.subscribe.bind(this);


Why do you need the binding? Seems like you are using arrow functions anyways.

take a look again, subscribe and unsubscribe are using react component's this

yeah, this is very implicit

createThemeListener() returns basically a mixin, it would be more explicit to merge that object with the prototype of withTheme class.

Object.assign(WithTheme.prototype, themeListener)

i dont like mixins, it allows one mixin to do too much and what is worse, allows it to do it in very implicit way

theme listener is low level api for cssinjs libs authors, shall it that implicit and "easy"?

I don't like them either. Though implementation detail over prototype here seems to me nicer/more clear than the bindings for each function.

theme listener is low level api for cssinjs libs authors, shall it that implicit and "easy"?

Interface is ok for now like that as we don't know it better. I am just talking about how you use it, Object.assign on prototype is more explicit to me than .bind on each function.

can you sketch the api surface?

we discussed it in gitter and decided it current api design is good enough

kof · 2017-05-22T09:03:51Z

src/create-with-theme.js


 function createWithTheme(CHANNEL = channel) {
+  const themeListener = createThemeListener(CHANNEL);
  return Component =>
    class withTheme extends React.Component {


class name should start with upper case

why? it works

sure, its just a convention everyone agreed on in javascript world. Class names upper case, instances lower.

withTheme lower case is the function you return on ln 7, which accepts a component, don't mix it up with the class name.

then, it will be withTheme(Comp) because its function, and WithTheme(Comp) in react dev tools, and its confusing. lets stick to withTheme because its even more against common practice to have a normal function (not a class) with capital letter

All of recomposes HoC are lowercased in both the function and the DevTools, so we're definitely fine going lowercase.

In Redux it is Connect(Something)
react-intl (very popular as well) - InjectIntl(Something)

After looking up other popular libs, UpperCase(Component) is def. a common way.

I don't care either way, let's not bikeshed on this for ages. This shouldn't block.

lets go with Capital letter then, but I will change it after pull-request will be merged

iamstarkov · 2017-05-22T10:30:41Z

@vesparny what do you think?

iamstarkov · 2017-05-23T13:50:51Z

@vesparny can you take a look?

kentcdodds · 2017-05-23T14:21:04Z

Maybe @kwelch would have an opinion here...

kwelch · 2017-05-23T14:40:08Z

Personally, I prefer the lowercase withTheme. One thing that was added in glamorous that I was a fan of was the merging of local theme with global theme. As it is written now if a theme prop is passed it will take precedence over the theme in context.

Here is how the merging is being done in glamorous. https://github.com/paypal/glamorous/blob/afbd1e1cf106ae46dd53b89015b46649bc833155/src/theme-provider.js#L17-L20

iamstarkov · 2017-05-23T18:51:16Z

@kwelch no, right now we merge outer theme and current one as well, i have tests for that

kwelch · 2017-05-23T18:57:31Z

Ah, thanks. I was only looking at within the scope of this PR. 😝

iamstarkov · 2017-05-31T21:00:26Z

accidentally closed this pull-request. will reopen it soon with updated implementation, tests and docs

coveralls · 2017-06-01T17:35:21Z

Coverage increased (+0.8%) to 93.277% when pulling abd34df on feat/theme-listener into 1a3e027 on dev-review.

kof · 2017-06-01T17:40:54Z

lgtm

coveralls · 2017-06-13T07:23:18Z

Coverage increased (+1.2%) to 93.636% when pulling 2a9e33f on feat/theme-listener into df0a20e on master.

kwelch · 2017-06-13T13:12:55Z

README.md

 * `createTheming` allows you to integrate `theming` into your CSSinJS library with custom `channel` (if you need custom one).
+=======


Merge conflict

thanks, fixed

kwelch · 2017-06-13T13:16:33Z

src/create-with-theme.js

-      };
+      constructor(props) {
+        super(props);
+        this.state = { theme: {} };


Should init theme be allowed to be passed via props?

i dunno, the same can easily be achieved by defaultProps of Component being enhanced

Good point. That works for me.

kwelch · 2017-06-13T13:18:18Z

src/index.js

 import defaultChannel from './channel';

 export const channel = defaultChannel;
 export const withTheme = createWithTheme();
 export const ThemeProvider = createThemeProvider();
+export const themeListener = createThemeListener();


Question: How can these exports be used since they all require a channel but they are invoked without one?

these exports are defaulted to default channel __theming__ internally

withTheme:

import channel from './channel'; export default function createWithTheme(CHANNEL = channel) { // … }

ThemeProvider:

import channel from './channel'; export default function createThemeProvider(CHANNEL = channel) { // … }

themeListener:

import channel from './channel'; export default function createThemeListener(CHANNEL = channel) { // … }

I see that now. Thanks, I knew it made sense, but I didn't see the connection.

coveralls · 2017-06-13T18:43:29Z

Coverage increased (+1.2%) to 93.636% when pulling 69836a0 on feat/theme-listener into 3cd9d5b on master.

iamstarkov · 2017-06-13T21:47:40Z

this branch has been published under @iamstarkov/theming-w-listener@1.0.1 if one want to try out themeListener integration

iamstarkov · 2017-06-14T21:35:19Z

i started to integrate it into jss, and it seems to be working. But themeListener needs to be adjusted a bit, but its a good news, because API surface will be smaller and simpler

iamstarkov · 2017-06-15T08:40:24Z

status update:

themeListener's code is updated
themeListener's docs are updated
withTheme implmentation is updated
themeListener's tests yet need to be updated, but hey they will become much more simpler

iamstarkov · 2017-06-15T08:43:50Z

no more bindings:

import { themeListener } from 'theming';

function CustomWithTheme(Component) {
  return class CustomWithTheme extends React.Component {
    static contextTypes = themeListener.contextTypes;
    constructor(props) {
      super(props);
      this.state = { theme: {} };
      this.setTheme = theme => this.setState({ theme });
    }
    componentWillMount() {
      this.setTheme(themeListener.initial(this.context))
    }
    componentDidMount() {
      this.unsubscribe = themeListener.subscribe(this.context, this.setTheme);
    }
    componentWillUnmount() {
      this.unsubscribe();
    }
    render() {
      const { theme } = this.state;
      return <Component theme={theme} {...this.props} />;
    }
  }
}

iamstarkov · 2017-06-15T08:47:02Z

published as @iamstarkov/theming-w-listener@1.0.2 if one want to try it out

jcheroske · 2017-06-21T00:12:35Z

What is the preferred method to map from the theme to props? For instance, if I wanted to pull stuff out of the theme and hand it to a MUI component, what would that look like? I'd like to see mapping from theme to props be a first-class operation. Something like:

import {mapTheme} from 'theming'

const mapThemeToProps = theme => ({
  underlineStyle: {
    color: theme.brandColors.color3
  }
})

const enhancer = mapTheme(mapThemeToProps)

export default enhancer(MyComponent)

Knowing that this lib will be embedded in react-jss, what I'd really like to see is the theme->props mapping functionality be elevated to a peer of the theme->jss-styles functionality. Not sure if injectSheet already takes in a second argument, but I've done something like this and it worked well:

import injectSheet from 'react-jss'

const mapThemeToStyles = theme => ({
  button: {
    color: theme.buttons.color
  }
}

const mapThemeToProps = theme => ({
  zDepth: theme.buttons.zDepth
})

const enhancer = injectSheet(mapThemeToStyles, mapThemeToProps)

const MyButton = ...

export default enhancer(MyButton)

Maybe this already exists in this PR?

iamstarkov · 2017-07-12T09:31:01Z

i tried this minimalistic abstraction in react-jss and it fits quite nicely

coveralls · 2017-07-12T10:58:46Z

Coverage increased (+0.9%) to 93.578% when pulling d0ccc43 on feat/theme-listener into 8ad0df2 on master.

kof · 2017-07-12T12:40:03Z

README.md

+```js
+import { themeListener } from 'theming';
+
+function CustomWithTheme(Component) {


I would call it createThemedComponent

kof

LGTM

iamstarkov mentioned this pull request May 21, 2017

docs, tests and implementation review #2

Merged

kof reviewed May 22, 2017

View reviewed changes

iamstarkov closed this May 29, 2017

iamstarkov force-pushed the feat/theme-listener branch from aa1cb57 to 1a3e027 Compare May 29, 2017 16:58

iamstarkov reopened this Jun 1, 2017

iamstarkov changed the base branch from dev-review to master June 12, 2017 19:19

iamstarkov mentioned this pull request Jun 12, 2017

Deprecate/remove ThemeProvider in favor of theming? paypal/glamorous#175

Closed

iamstarkov force-pushed the feat/theme-listener branch from abd34df to 79a07f9 Compare June 13, 2017 07:01

kwelch reviewed Jun 13, 2017

View reviewed changes

iamstarkov force-pushed the feat/theme-listener branch from a536d30 to 69836a0 Compare June 13, 2017 18:34

iamstarkov mentioned this pull request Jun 15, 2017

Opt out of error if there is no theme context in the tree #13

Closed

kentcdodds mentioned this pull request Jun 15, 2017

Allow users to have you call their component as a function instead of createElement #14

Closed

iamstarkov added 11 commits July 12, 2017 11:34

write documentation for themeListener

f5c848e

write tests for themeListener

186c65d

implement theme-listener

e884f5f

redo withTheme via themeListener

fa242d7

expose themeListener

2e5b143

fix: convert themeListener to esm

b9887e8

docs: fix merge conflict

9204802

refactor: simplify themeListener

5b9a982

update withTheme

f895b97

docs: update documentation for themeListener

e750397

docs: overall themeListener documentation updated

b48e72f

iamstarkov force-pushed the feat/theme-listener branch from 9b62882 to b48e72f Compare July 12, 2017 09:34

iamstarkov added 2 commits July 12, 2017 12:50

finish theme-listener

e24a642

docs: prepare changelog

d0ccc43

kof reviewed Jul 12, 2017

View reviewed changes

kof approved these changes Jul 12, 2017

View reviewed changes

iamstarkov merged commit def27be into master Jul 12, 2017

iamstarkov deleted the feat/theme-listener branch July 12, 2017 15:03

		* `createTheming` allows you to integrate `theming` into your CSSinJS library with custom `channel` (if you need custom one).
		=======

Feature: theme listener #3

Feature: theme listener #3

Conversation

iamstarkov commented May 21, 2017 • edited Loading

coveralls commented May 21, 2017 • edited Loading

iamstarkov commented May 21, 2017

iamstarkov commented May 21, 2017

kentcdodds commented May 21, 2017

mxstbr commented May 22, 2017

iamstarkov commented May 22, 2017

iamstarkov commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof May 22, 2017 • edited Loading

Choose a reason for hiding this comment

iamstarkov May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof May 22, 2017 • edited Loading

Choose a reason for hiding this comment

kof May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamstarkov commented May 22, 2017

iamstarkov commented May 23, 2017

kentcdodds commented May 23, 2017

kwelch commented May 23, 2017

iamstarkov commented May 23, 2017

kwelch commented May 23, 2017

iamstarkov commented May 31, 2017

coveralls commented Jun 1, 2017 • edited Loading

kof commented Jun 1, 2017

coveralls commented Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamstarkov Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamstarkov Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2017 • edited Loading

iamstarkov commented Jun 13, 2017

iamstarkov commented Jun 14, 2017

iamstarkov commented Jun 15, 2017

iamstarkov commented Jun 15, 2017

iamstarkov commented Jun 15, 2017

jcheroske commented Jun 21, 2017

iamstarkov commented Jul 12, 2017

coveralls commented Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

kof left a comment

Choose a reason for hiding this comment

iamstarkov commented May 21, 2017 •

edited

Loading

coveralls commented May 21, 2017 •

edited

Loading

kof May 22, 2017 •

edited

Loading

iamstarkov May 22, 2017 •

edited

Loading

kof May 22, 2017 •

edited

Loading

kof May 22, 2017 •

edited

Loading

coveralls commented Jun 1, 2017 •

edited

Loading

coveralls commented Jun 13, 2017 •

edited

Loading

iamstarkov Jun 13, 2017 •

edited

Loading

iamstarkov Jun 13, 2017 •

edited

Loading

coveralls commented Jun 13, 2017 •

edited

Loading

coveralls commented Jul 12, 2017 •

edited

Loading