Convert react-dates to use react-with-styles #722

majapw · 2017-09-22T20:48:16Z

This change moves react-dates onto Airbnb's modern best practices of CSS in your Javascript!

Because this is again a huge change, it is best viewed on a commit by commit level (which I've tried to split up by component).

While this transformation of the code is complete, there are still a number of things to be done.

Documentation! How do you use react-with-styles?
More customization! I think we should identify overrides that might be useful in the theme since you can't go about overriding everything willy-nilly anymore
~~I think we might want to export a CSS-styles version of the code as well as the vanilla version~~
TESTS DON'T WORK ANYMORE. I need to fix that.
Once tests are running, probably some of them will be broken
there is a weird flash before the height of the calendar month grid is set now... we should address that.

to: @ljharb @airbnb/webinfra @lencioni @adamrneary @sdjidjev

coveralls · 2017-09-22T20:57:12Z

Coverage decreased (-83.4%) to 3.06% when pulling fc964d6 on maja-implement-with-styles into c464d43 on master.

coveralls · 2017-09-29T00:23:05Z

Coverage increased (+0.6%) to 86.994% when pulling d3d5b3a on maja-implement-with-styles into 0f6cccc on master.

lencioni

I haven't looked at the diff very closely yet, but this change should probably also update documentation to help people customize, right?

majapw · 2017-09-29T16:52:56Z

@lencioni yup! That is on my TODO. :)

coveralls · 2017-10-02T23:52:28Z

Coverage increased (+0.2%) to 86.53% when pulling 52de5ce on maja-implement-with-styles into 0f6cccc on master.

gabergg · 2017-10-03T21:25:39Z

src/components/CalendarDay.jsx

@@ -118,12 +116,44 @@ export default class CalendarDay extends React.Component {
      height: daySize - 1,
    };

+    const useDefaultCursor =


As we discussed, maybe worth converting these modifiers to boolean properties or something similar. Might be an easier interface + more intuitive.

I think we can do this in a follow-up PR (and one that is slightly less overwhelming to read)

gabergg · 2017-10-03T21:58:01Z

src/shapes/withStylesPropTypes.js

@@ -0,0 +1,8 @@
+import PropTypes from 'prop-types';


Why pull this out of react-with-styles?

yeah that's actually a good question; can't we import from there instead?

react-with-styles doesn't actually have this file. :P We fake it in our own internal repo. Probably the right thing to do is to add it to react-with-styles

aha, yes let's please do that asap :-D

gabergg · 2017-10-03T22:04:42Z

src/components/CalendarDay.jsx

@@ -174,6 +174,7 @@ class CalendarDay extends React.Component {
 CalendarDay.propTypes = propTypes;
 CalendarDay.defaultProps = defaultProps;

+export { CalendarDay as PureCalendarDay };


Could Pure be a misleading term here? Maybe overloaded

wait, why are we exporting this at all? best practice is to only use .dive() to get at the unwrapped component

because we are spying on prototype methods. dive doesn't solve that issue afaik, does it?

you have me there.

any way we could avoid the spying?

it'd be ugly but we could const Pure = shallow(<Wrapped />).dive().type(); sinon.spy(Pure.prototype, 'foo')

ljharb · 2017-10-03T21:43:01Z

.storybook-css/config.js

+
+addDecorator((story) => {
+  moment.locale('en');
+  return (story());


why the parens?

ljharb · 2017-10-03T21:44:50Z

.storybook-css/config.js

+        overflow: 'scroll',
+      }}
+    >
+      <span dangerouslySetInnerHTML={{ __html: helperText }} />


why inside the span, and not directly inside the div?

ljharb · 2017-10-03T21:47:53Z

.storybook/webpack.config.js

@@ -4,10 +4,9 @@ module.exports = {
  module: {
    rules: [
      {
-        test: /\.scss$/,
+        test: /(\.scss$|\.css$)/,


/\.s?css$/? (if we're going to use the parens, we'd want (?: to make the group non-capturing)

ljharb · 2017-10-03T21:52:25Z

css/styles.css

@@ -0,0 +1,18876 @@
+.DateRangePicker {
+  position: relative;
+  display: inline-block


super sad that this output lacks a final semicolon

This file is autogenerated and that process still needs some improvements --- namely on collapsing style definitions as well as best practices for styling. X_x

I think I have also committed the unoptimized version here which I ... probably don't want?

I think I should just .gitignore this file tbh.

yeah that's a good idea, good call

ljharb · 2017-10-03T22:02:14Z

css/styles.css

+  direction: rtl
+}
+.DateRangePicker_picker__directionLeft {
+  left: 0px


also that zero values have units

again this is due partly to the autogeneration of this file and i need to... probably clean it up a bit more/read more into clean-css options.

ljharb · 2017-10-03T22:23:54Z

src/components/DayPicker.jsx

+    }
+
+    const verticalScrollable = orientation === VERTICAL_SCROLLABLE;
+    if (verticalScrollable) firstVisibleMonthIndex = 0;


if (orientation === VERTICAL_SCROLLABLE) return 0;?

ljharb · 2017-10-03T22:24:59Z

src/components/DayPicker.jsx

+    this.calendarMonthGridHeight = calendarMonthHeights.reduce((maxHeight, height, i) => {
+      const isVisible =
+        (i >= firstVisibleMonthIndex) && (i < firstVisibleMonthIndex + numberOfMonths);
+      return isVisible ? Math.max(maxHeight, height) : maxHeight;


Math.max(...calendarMonthHeights.filter((, i) => (i >= firstVisibleMonthIndex) && (i < firstVisibleMonthIndex + numberOfMonths))?

ljharb · 2017-10-03T22:25:47Z

src/components/DayPickerKeyboardShortcuts.jsx

@@ -128,42 +105,61 @@ export default class DayPickerKeyboardShortcuts extends React.Component {
      ? phrases.hideKeyboardShortcutsPanel
      : phrases.showKeyboardShortcutsPanel;

+    const bottomRight = (buttonLocation === BOTTOM_RIGHT);


all these parens are redundant

ljharb · 2017-10-03T22:27:54Z

src/utils/calculateDimension.js

+  // Get the inner size
+  if (!borderBox) {
+    size -= (
+      parseFloat(style[`padding${axisStart}`]) +


does this need to pivot based on RTL?

I do not believe so (or at the least, react-dates currently still works as expected with the isRTL prop). However, I am pegging to the old version of RWS-IA for this change and will come back to the question of RTL support with the new interface in a follow-up PR. :)

ljharb · 2017-10-03T22:28:32Z

stories/DateRangePicker.js

@@ -83,7 +83,6 @@ storiesOf('DateRangePicker (DRP)', module)
    moment.locale('fa');
    return (
      <DateRangePickerWrapper
-        placeholder="تقویم فارسی"


isn't the goal here to have farsi/arabic in the example? why remove the placeholder?

This didn't actually do anything. placeholder is not a valid prop on the DRP and the text itself translates to Farsi Calendar which is not a meaningful value for the startDatePlaceholderText or the endDatePlaceholderText props.

majapw · 2017-10-04T22:11:42Z

@ljharb @gabergg I believe I've addressed all of your comments.

majapw · 2017-10-04T22:12:41Z

src/theme/DefaultTheme.js

+  yellow_dark: '#ffce71',
+};
+
+export default {


One of the things I was thinking about was that... if we're importing this into a project that uses many react-with-styles packages and we need to register the theme, we should probably namespace the theme so that there are no collisions. Something like:

export default { reactDates: { color: ..., zIndex: ..., ... }, };

@gabergg @lencioni @ljharb what do you think?

Yep, I think it should definitely be namespaced.

I'm confused, why should it be namespaced?

This module exports a theme directly; wouldn't whoever registered the theme be able to install it under a key themselves?

But the components themselves would be expecting the color, zIndex, etc. keys directly and there'd be no way to change that.

gotcha. maybe it'd be better to have withStyles support a "namespace" option or something, but this is fine for now i guess.

coveralls · 2017-10-04T22:15:48Z

Coverage increased (+0.3%) to 86.695% when pulling 5ec30b3 on maja-implement-with-styles into a7747ea on master.

coveralls · 2017-10-04T22:20:27Z

Coverage increased (+0.3%) to 86.695% when pulling 5ec30b3 on maja-implement-with-styles into a7747ea on master.

coveralls · 2017-10-04T22:55:56Z

Coverage increased (+0.3%) to 86.695% when pulling 4407874 on maja-implement-with-styles into a7747ea on master.

coveralls · 2017-10-04T23:01:50Z

Coverage increased (+0.3%) to 86.695% when pulling 4407874 on maja-implement-with-styles into a7747ea on master.

ljharb · 2017-10-04T22:47:12Z

.storybook-css/addons.js

@@ -0,0 +1,4 @@
+/* eslint-disable import/no-extraneous-dependencies, import/extensions */


why are these deps extraneous?

They're never used directly!

(but they are necessary)

They're imported here; "extraneous" means they're not in package.json. If they're necessary, why can't they be in package.json?

So the thing is... they come with @storybook/react but they have namespacing that makes eslint think they're separate packages. idk man, i blame storybook for this one, but this is what you have to do to get them to work.

aren't they separate packages? https://www.npmjs.com/package/@storybook/addon-actions and https://www.npmjs.com/package/@storybook/addon-links

I don't believe they were when I first put together this addons.js file/updated storybook in this package. It's possible this has changed in newer versions.

This is literally just a copy of what's in .storybook which is the default config.

ljharb · 2017-10-04T22:48:52Z

scripts/buildCSS.js

+const fs = require('fs');
+const CleanCSS = require('clean-css');
+
+const CSSInterface = require('react-with-styles-interface-css').default;


wait; this is a bug in the css interface. There should never be a default property on module.exports for a package's entry point :-/

It's possible your addition of registerMaxSpecificity caused this; if so, maybe we should remove it and have it be under a different entry point (require('react-with-styles-interface-css/registerMaxSpecificity'))

airbnb/react-with-styles-interface-css#8 should fix some things!

ljharb · 2017-10-04T22:50:01Z

scripts/buildCSS.js

+
+require('../test/_helpers/ignoreSVGStrings');
+
+registerMaxSpecificity(11);


why is this turned up to eleven?

(no but seriously, why the magic number?)

I should leave a comment. The number comes from the maximum number of styles passed into any css call in this repo

ljharb · 2017-10-04T22:50:33Z

scripts/buildCSS.js

+registerMaxSpecificity(11);
+registerInterfaceWithDefaultTheme(CSSInterface);
+
+const DateRangePickerPath = './src/components/DateRangePicker.jsx';


don't you want to pull in the babel output, not the raw code?

I don't... think so. At least, that's not what the css interface uses as its tests.

ljharb · 2017-10-04T22:50:37Z

scripts/buildCSS.js

+
+const CSSInterface = require('react-with-styles-interface-css').default;
+const registerMaxSpecificity = require('react-with-styles-interface-css').registerMaxSpecificity;
+const compileCSS = require('react-with-styles-interface-css/compile').default;


Same airbnb/react-with-styles-interface-css#8

ljharb · 2017-10-04T22:53:22Z

src/components/CalendarMonthGrid.jsx

+  setMonthHeight(height, i) {
+    if (this.calendarMonthHeights[i]) {
+      if (i === 0) {
+        this.calendarMonthHeights = [height, ...this.calendarMonthHeights.slice(0, -1)];


it's slightly more efficient here to do [height].concat(this.calendarMonthHeights.slice(0, -1))

ljharb · 2017-10-04T22:59:18Z

src/components/DayPicker.jsx

@@ -898,3 +813,110 @@ export default class DayPicker extends React.Component {

 DayPicker.propTypes = propTypes;
 DayPicker.defaultProps = defaultProps;
+
+export { DayPicker as PureDayPicker };


i think i called this out before; this is just to spy on instance methods in tests, yes?

ljharb · 2017-10-04T23:00:30Z

src/components/SingleDatePicker.jsx

@@ -479,3 +472,82 @@ export default class SingleDatePicker extends React.Component {

 SingleDatePicker.propTypes = propTypes;
 SingleDatePicker.defaultProps = defaultProps;
+
+export { SingleDatePicker as PureSingleDatePicker };


ljharb · 2017-10-04T23:02:09Z

src/theme/DefaultTheme.js

+  yellow_dark: '#ffce71',
+};
+
+export default {


I'm confused, why should it be namespaced?

This module exports a theme directly; wouldn't whoever registered the theme be able to install it under a key themselves?

ljharb · 2017-10-04T23:03:06Z

test/.eslintrc

    "indent": [2, 2, {
-		"MemberExpression": "off"
-	}],
+  		"MemberExpression": "off"


these two lines have tab chars; please make them spaces

coveralls · 2017-10-05T00:59:06Z

Coverage increased (+0.3%) to 86.695% when pulling eab055e on maja-implement-with-styles into a7747ea on master.

majapw · 2017-10-05T18:36:56Z

@ljharb @gabergg @lencioni I am fishing for a stamp! :) I think I have addressed all comments.

coveralls · 2017-10-05T18:49:07Z

Coverage increased (+0.3%) to 86.695% when pulling 312cdd0 on maja-implement-with-styles into a7747ea on master.

ljharb · 2017-10-05T21:42:31Z

scripts/buildCSS.js

+
+const compileCSS = require('react-with-styles-interface-css/compile');
+
+const registerMaxSpecificity = require('react-with-styles-interface-css/dist/utils/registerMaxSpecificity').default;


oh noes, there's still dot defaults? :-(

…use react-with-styles

coveralls · 2017-10-05T22:43:29Z

Coverage increased (+0.3%) to 86.733% when pulling 177bf03 on maja-implement-with-styles into b737a8e on master.

majapw · 2018-03-20T20:54:13Z

@adamchenwei this PR was merged in in October and has been in active use since then... Have you read through the README, specifically this section: https://github.com/airbnb/react-dates#initialize?

There's also a bit more context on the transition to react-with-styles in https://github.com/airbnb/react-dates#advanced.

I'd love to answer more specific questions if you have them in a new issue. :)

majapw · 2018-03-21T03:15:58Z

@adamchenwei can you please open an issue so we can continue this discussion there where it's a bit more discoverable? :)

majapw force-pushed the maja-implement-with-styles branch 2 times, most recently from b8cf807 to d3d5b3a Compare September 29, 2017 00:13

lencioni reviewed Sep 29, 2017

View reviewed changes

gabergg reviewed Oct 3, 2017

View reviewed changes

ljharb reviewed Oct 3, 2017

View reviewed changes

ljharb mentioned this pull request Oct 4, 2017

Would you accept a PR migrating node-sass to css-modules? #745

Closed

majapw force-pushed the maja-implement-with-styles branch 2 times, most recently from b9e79be to 5ec30b3 Compare October 4, 2017 22:06

majapw commented Oct 4, 2017

View reviewed changes

majapw force-pushed the maja-implement-with-styles branch 2 times, most recently from 882b2b3 to 4407874 Compare October 4, 2017 22:45

ljharb reviewed Oct 4, 2017

View reviewed changes

majapw force-pushed the maja-implement-with-styles branch from 4407874 to eab055e Compare October 5, 2017 00:49

majapw force-pushed the maja-implement-with-styles branch from eab055e to 312cdd0 Compare October 5, 2017 18:36

ljharb previously approved these changes Oct 5, 2017

View reviewed changes

Convert CalendarDay to use react-with-styles

e2c391d

Maja Wichrowska added 19 commits October 5, 2017 15:10

Move KeyboardShortcutRow onto react-with-styles

c14c62b

Convert DayPickerKeyboardShortcuts to react-with-styles

608e55d

Convert DayPicker to use react-with-styles

6c43cf9

Fix height adjustment on month transitions

30941f8

Get initial month height set

a137bb1

Get initial month position set correctly when not animating

295c3d3

Fix month transitions

64cf972

Get height adjustment working in the DayPicker

83c4fa7

Fix verticalScrollable datepicker

2afc06d

Ensure DayPicker*Controller components still work

162a0c5

Create withStylesPropTypes shape and import that everywhere

86641d2

Convert DateInput, DateRangePickerInput and SingleDatePickerInput to …

2710b97

…use react-with-styles

Move DRP and SDP onto react-with-styles

3627340

Clean up left over errors and warnings

e724d12

Get tests running and also fix them

9515fea

Get CSS interface working with react-dates

dd530bf

Fix month height issues and transitioning in rtl

ef6a2b2

Fix up package.json scripts and add documentation

f3e731e

Address PR comments

177bf03

majapw dismissed ljharb’s stale review via 177bf03 October 5, 2017 22:36

majapw force-pushed the maja-implement-with-styles branch from 312cdd0 to 177bf03 Compare October 5, 2017 22:36

ljharb approved these changes Oct 5, 2017

View reviewed changes

majapw merged commit a569622 into master Oct 5, 2017

majapw deleted the maja-implement-with-styles branch October 5, 2017 23:45

This comment has been minimized.

Sign in to view

		@@ -0,0 +1,4 @@
		/* eslint-disable import/no-extraneous-dependencies, import/extensions */


		require('../test/_helpers/ignoreSVGStrings');

		registerMaxSpecificity(11);


		const compileCSS = require('react-with-styles-interface-css/compile');

		const registerMaxSpecificity = require('react-with-styles-interface-css/dist/utils/registerMaxSpecificity').default;

Convert react-dates to use react-with-styles #722

Convert react-dates to use react-with-styles #722

Conversation

majapw commented Sep 22, 2017 • edited by ljharb Loading

coveralls commented Sep 22, 2017

coveralls commented Sep 29, 2017

lencioni left a comment

Choose a reason for hiding this comment

majapw commented Sep 29, 2017

coveralls commented Oct 2, 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

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

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

majapw commented Oct 4, 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

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 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

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

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

coveralls commented Oct 5, 2017

majapw commented Oct 5, 2017

coveralls commented Oct 5, 2017

Choose a reason for hiding this comment

coveralls commented Oct 5, 2017

This comment has been minimized.

majapw commented Mar 20, 2018

This comment has been minimized.

This comment has been minimized.

majapw commented Mar 21, 2018

majapw commented Sep 22, 2017 •

edited by ljharb

Loading