Add directional `StyleSheet` creation based on context #117

majapw · 2017-11-02T17:47:04Z

This change introduces a dependency on react-with-direction (https://github.com/airbnb/react-with-direction) although in the actual dependencies, we only make use of the constants from that package. The idea is that if you wanted to leverage the LTR/RTL capabilities of react-with-styles, you would pair it with a DirectionProvider at the top-level. Introducing those constants as part of the react-with-styles package makes that relationship explicit.

If you want to continue using react-with-styles as is, this would not add terribly much weight and the default fallbacks would all still work as expected.

The next step would be to add directional StyleSheet creation to our interfaces.

From #115:

After some offline conversation about the best approach to handling specificity issues in react-with-styles-interface-aphrodite/with-rtl, we arrived at a three step approach that would eliminate specificity issue entirely.

In order to be able to modify built-in styles based on context, defer stylesheet creation to the constructor of the withStyles HOC. This will also give us the performance benefit of not calling StyleSheet.create for currently unrendered components (but will also slow down the first render a little bit)

Based on directional context, cache and create the appropriate StyleSheet in the constructor of the withStyles HOC.

Pass a directional css method down to the wrapped component as a prop. This will be a breaking change and will require that components use props.css instead of the react-with-styles export. We will initially address this change internally using a babel plugin and eventually do a sweeping codemod. The directional css method will flip or not flip inline styles depending on context --- this will necessitate an update to RWS-IA.

This is step 2 of the process.

@ljharb @lencioni @yzimet @garrettberg

ljharb · 2017-11-02T19:04:33Z

package.json

@@ -78,6 +79,7 @@
    "deepmerge": "^1.5.2",
    "global-cache": "^1.2.1",
    "hoist-non-react-statics": "^2.3.1",
-    "prop-types": "^15.6.0"
+    "prop-types": "^15.6.0",
+    "react-with-direction": "^1.1.0"


let's make this both a dep and a peer dep - a peer dep so there's no conflict in case they use a different version of react-with-direction, and "also a dep" so npm 3+ users don't have to manually add it to package.json.

ljharb · 2017-11-02T19:05:11Z

src/ThemedStyleSheet.js

@@ -59,7 +67,8 @@ export default globalCache.setIfMissingThenGet(
  () => ({
    registerTheme,
    registerInterface,
-    create,
+    create: createLTR,
+    createRTL,


should we also add createLTR, here, for symmetry?

I don't know how much sense that makes if create always defaults to createLTR and I wanna keep create for the default case (and for backwards compatibility)

yzimet

LGTM, exciting!

yzimet · 2017-11-02T23:54:31Z

src/withStyles.jsx

+        const isRTL = direction === DIRECTIONS.RTL;
+        if (isRTL && !styleDefRTL) {
+          styleDefRTL = styleFn ? ThemedStyleSheet.createRTL(styleFn) : EMPTY_STYLES_FN;
+        } else if (!styleDefLTR) {


nit: add !isRTL to the condition for clarity

yzimet · 2017-11-03T00:01:42Z

test/withStyles_test.jsx

+      );
+      expect(testInterface.createRTL.callCount).to.equal(1);
+    });
+  });


nit: please add one more test for a <WrappedComponent /> that is not within a DirectionProvider?

Whoops that's what I meant the first test to be :P

majapw · 2017-11-03T19:53:46Z

@ljharb @yzimet PTAL!

yzimet

LGTM!

ljharb · 2017-11-04T02:11:59Z

src/withStyles.jsx

@@ -28,6 +31,10 @@ function baseClass(pureComponent) {
  return React.Component;
 }

+const contextTypes = {
+  [CHANNEL]: brcastShape,


it's unfortunate we're not using withDirection here :-/

I mean it would add an extra virtual DOM node to ... every single component in our tree... which doesn't seem good. :/

Maybe we could export more things from react-with-direction to avoid as much duplication as possible?

Perhaps we could export directionProviderContextTypes? the grabbing of the direction from the context is also a bit gross.

I'll think about implementing this in a follow up

Yes, we should definitely do that at a minimum.

ljharb · 2017-11-04T02:12:44Z

test/withStyles_test.jsx

+          <WrappedComponent />
+        </DirectionProvider>,
+      );
+      expect(testInterface.createRTL.callCount).to.equal(1);


expect(testInterface.createRTL).to.have.property('callCount', 1) will have a nicer failure message.

majapw added the semver-minor: new stuff label Nov 2, 2017

ljharb reviewed Nov 2, 2017

View reviewed changes

yzimet approved these changes Nov 3, 2017

View reviewed changes

majapw force-pushed the maja-add-contextual-styling branch from 952296c to c196ce1 Compare November 3, 2017 19:53

yzimet approved these changes Nov 3, 2017

View reviewed changes

ljharb approved these changes Nov 4, 2017

View reviewed changes

Add directional stylesheet creation based on context

b875bd0

majapw force-pushed the maja-add-contextual-styling branch from c196ce1 to b875bd0 Compare November 6, 2017 18:42

majapw merged commit 204b27f into master Nov 6, 2017

majapw deleted the maja-add-contextual-styling branch November 6, 2017 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add directional `StyleSheet` creation based on context #117

Add directional `StyleSheet` creation based on context #117

majapw commented Nov 2, 2017

ljharb Nov 2, 2017

ljharb Nov 2, 2017

majapw Nov 2, 2017

yzimet left a comment

yzimet Nov 2, 2017

yzimet Nov 3, 2017

majapw Nov 3, 2017

majapw commented Nov 3, 2017

yzimet left a comment

ljharb Nov 4, 2017

majapw Nov 6, 2017

lencioni Nov 6, 2017

majapw Nov 6, 2017

majapw Nov 6, 2017

ljharb Nov 6, 2017

ljharb Nov 4, 2017

Add directional StyleSheet creation based on context #117

Add directional StyleSheet creation based on context #117

Conversation

majapw commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yzimet left a 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

majapw commented Nov 3, 2017

yzimet left a 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

Add directional `StyleSheet` creation based on context #117

Add directional `StyleSheet` creation based on context #117