Send css method down as a prop instead of as a global

[majapw]

In order to solve some problems we've seen with specificity, as well as some server-side/client-side disparities, we will be attempting to separate out RTL and LTR styles entirely. Part of this has been accomplished by splitting StyleSheet creation into RTL and LTR. The next step is to send down a directional CSS method as a prop.

I've left in the resolve method as is... but I'm unsure if that is necessary. Should we still export it from the ThemedStyleSheet or the withStyles file?

to: @yzimet @lencioni @garrettberg @ljharb @airbnb/webinfra

[garrettberg]

I've left in the resolve method as is... but I'm unsure if that is necessary. Should we still export it from the ThemedStyleSheet or the withStyles file?

Not sure it makes sense to keep around in this form. Should this maybe mimic create where the exported create is just the LTR version?

[garrettberg]

Nit: Maybe just make this isRTL() since that's how it's used in both places below?

const direction = this.context[CHANNEL] && this.context[CHANNEL].getState();

return direction === DIRECTIONS.RTL;

[yzimet]

Looks great!

I've left in the resolve method as is... but I'm unsure if that is necessary. Should we still export it from the ThemedStyleSheet or the withStyles file?

Seems unnecessary, since the resolveLTR/RTL methods fallback to it internally. I guess it depends if we want this to be a breaking change or not.

Let's update the specs, too!

[ljharb]

this makes it a breaking change, fwiw.

[ljharb]

as does this

[majapw]

@ljharb @yzimet @garrettberg PTAL! Also this pairs with airbnb/react-with-styles-interface-aphrodite#37.

I think what I have in mind for the upgrade path is that by leaving the (non-directional) css export in, we are able to upgrade in chunks (as in a folder at a time) which seems beneficial. I'm going to couple this with a lint rule that prevents importing of the css file directly and a codemod that goes through and updates all files to use the prop instead. WDYT?

[lencioni]

In order to solve some problems we've seen with specificity, as well as some server-side/client-side disparities

are these problems detailed somewhere?

[garrettberg]

LGTM!

Upgrade plan sounds good too.

[yzimet]

LGTM!

[yzimet]

could you remind me, did we consider using the withDirection HOC directly instead of reimplementing this?

[majapw]

I considered it, but adding the extra virtual DOM node seemed possibly problematic. We can go back and do that in a patch change if that makes more sense moving forward.

I did think it was sad to duplicate this code.... so maybe the right thing to do would be to wrap?

I'll defer to @lencioni when he is back from PTO.

[lencioni]

Because this HOC is used so prolifically throughout a tree, I would be hesitant to drop in another component into the mix. If you wanted to keep it DRY you could probably export some utility methods from withDirection and then use them here.

[ljharb]

That’d be a great follow up, i had the same thought

[majapw]

To close the loop on @lencioni's comment, here is a description of why we're doing this change:

In November 2017, we had a pretty much fully-functional RTL solution for the web. There were a few quirks depending on how consumers were writing their CSS and icons/graphics still needed to be updated, but for the most part, pages were functional. However, this solution neglected one aspect of the problem (nested directional tags) and additionally, became less stable when server/client mismatches were highlighted by the React 16 upgrade (ie no longer failed gracefully). So the problems with the initial solution were:

1. The solution relied on descendent selectors, which basically meant that a top-level [dir=”rtl”] or [dir=”ltr] determined all of the styles for its children. However, if one of the child nodes also contained a conflicting dir attribute, then both directional styles would be applied to everything below that point. For the most part, user generated content is just text so this may not practically be as huge of an issue, but anything more complex (with margins, absolute positioning, paddings, etc) would suffer from this strange behavior. There is no way in CSS to apply a :not selector to descendents so this would never be a complete solution.

2. The React 16 upgrade highlighted an issue where there appeared to be a difference in the styles applied between server and client. This appeared to happen in particular when dealing with matchmedia (responsive) queries, which suggests that perhaps that when there was a mismatch between what screensize the server guessed the user was on and the actual screen size of the user, there was an effect on the styles rendered.

This V2 solution does not rely on descendent selectors at all, but instead, passes down a directional css prop which will calculate the necessary styles based on the directional context.