Custom Input Icon--calendar position by mariepw · Pull Request #627 · react-dates/react-dates

mariepw · 2017-07-18T15:40:19Z

Hi @majapw, Hi all,
I implemented the changes proposed in #625 by adding prop 'showInputIconRight' to 'SingleDatePicker' and 'DateRangePicker'.

coveralls · 2017-07-18T15:48:43Z

Coverage increased (+0.04%) to 86.283% when pulling 3430bdf on mariepw:CustomPositionIcon into f149c68 on airbnb:master.

coveralls · 2017-07-18T16:12:29Z

Coverage increased (+0.04%) to 86.283% when pulling 3a13698 on mariepw:CustomPositionIcon into f149c68 on airbnb:master.

ljharb

I'm also wondering if perhaps we want an enum for inputIconPosition that can be either "before" or "after", rather than a boolean?

ljharb · 2017-07-18T16:45:52Z

  screenReaderInputMessage: '',
  showClearDates: false,
  showDefaultInputIcon: false,
+  showInputIconRight: false,


Let's avoid using terms like "Right" - in an RTL world, "after" is more appropriate.

ljharb · 2017-07-18T16:46:57Z

  }

+  renderInputIcon(inputIcon) {
+    return (<button


the ( should end the line, and the ) should begin the line - iow:

return ( <button type="button" className="DateRangePickerInput__calendar-icon" disabled={this.props.disabled} aria-label={this.props.phrases.focusStartDate} onClick={this.props.onArrowDown} > {inputIcon} </button> );

Also, please destructure all props at the top of the function.

ljharb · 2017-07-18T16:47:17Z

  }

+  renderInputIcon(inputIcon) {
+    return (<button


indentation and prop destructuring here also

mariepw · 2017-07-19T08:35:32Z

Thanks for your review, I've done the requested changes.

ljharb

LGTM pending the last few comments.

ljharb · 2017-07-19T08:42:49Z

  readOnly: PropTypes.bool,
  showCaret: PropTypes.bool,
  showDefaultInputIcon: PropTypes.bool,
+  inputIconPosition: PropTypes.oneOf('before', 'after'),


this should come from the shape, so that it doesn't have to be defined more than once

DateRangePicker imports shape, but DateRangePickerInput and DateRangePickerInputController don't.. Do I have to change the complete structure?

I'm not sure you need to refactor much; newly importing the shape should be fine.

SimpleDatePickerShape/SimpleDatePicker and DateRangePickerShape/DateRangePicker share same props..
But DateRangePickerInputController SimpleDatePicker DateRangePickerInput, they all have their own props, which are quite different from parents [..Shape and ..Picker]..
I think shape files have props that input ones don't need..
But maybe I miss something, will investigate a bit more..

There's a couple alternatives here; one is importing the shape but only extracting the one propType - another is putting this propType in its own file, and importing that in all the places it's needed. I'd prefer that approach tbh :-)

ljharb · 2017-07-19T08:43:12Z

  }

+  renderInputIcon(inputIcon) {
+    const {disabled, phrases.focusStartDate, onArrowDown} = this.props;


curly braces in JS need padding spaces inside them - the linter should be failing here.

ljharb · 2017-07-19T08:43:33Z

  }

+  renderInputIcon(inputIcon) {
+    const {disabled, phrases.focusStartDate, onFocus} = this.props;


coveralls · 2017-07-19T10:01:32Z

Coverage increased (+0.06%) to 86.303% when pulling 8a62d98 on mariepw:CustomPositionIcon into f149c68 on airbnb:master.

…an instance of array'

coveralls · 2017-07-19T10:23:48Z

Coverage increased (+0.06%) to 86.303% when pulling 05a0d96 on mariepw:CustomPositionIcon into f149c68 on airbnb:master.

majapw

I think we should pull before, after, and the prop type into reusable variables. :) I also have a few nits about code duplication. O/w this is looking great!

majapw · 2017-07-19T20:54:35Z

  screenReaderInputMessage: '',
  showClearDates: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


we probably want to pull this 'before'/'after' into the constants file

Now that we have the constants file, it'd be better to import that and use BEFORE_POSITION here.

majapw · 2017-07-19T20:55:07Z

  readOnly: PropTypes.bool,
  showCaret: PropTypes.bool,
  showDefaultInputIcon: PropTypes.bool,
+  inputIconPosition: PropTypes.oneOf(['before', 'after']),


Since we reuse this, we probably want to save this as a shape, similar to OrientationShape

majapw · 2017-07-19T21:01:27Z

-            {inputIcon}
-          </button>
+
+        {(inputIconPosition === 'before' && (showDefaultInputIcon || customInputIcon !== null)) && (


We could probably pull (showDefaultInputIcon || customInputIcon !== null) && this.renderInputIcon(inputIcon) into a variable above the return and then just have inputIconPosition === 'before' && inputIcon

majapw · 2017-07-19T21:01:48Z

-          >
-            {inputIcon}
-          </button>
+        {(inputIconPosition === 'before' && (showDefaultInputIcon || customInputIcon !== null)) && (


same comment applies here

@majapw

LGTM, deferring to @majapw

mariepw · 2017-07-20T07:34:36Z

Thanks @majapw and @ljharb.
Changes done !

…on position

coveralls · 2017-07-20T07:43:49Z

Coverage increased (+0.08%) to 86.324% when pulling f8c583c on mariepw:CustomPositionIcon into f149c68 on airbnb:master.

mariepw · 2017-07-24T08:18:42Z

Build doesn't seem to fail because of my changes...
It seems like it's a npm error

npm ERR! errno 1
npm ERR! react-dates@12.3.0 tests-karma: `karma start`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the react-dates@12.3.0 tests-karma script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2017-07-20T07_36_39_505Z-debug.log

majapw · 2017-07-24T16:18:53Z

@ljharb this actually appears to be failing due to some extraneous dependencies being installed after we attempt to install the relevant react version:

npm ERR! extraneous: babel-plugin-react-docgen@1.5.0 /home/travis/build/airbnb/react-dates/node_modules/babel-plugin-react-docgen
npm ERR! extraneous: css-loader@0.28.4 /home/travis/build/airbnb/react-dates/node_modules/css-loader
npm ERR! extraneous: cssesc@0.1.0 /home/travis/build/airbnb/react-dates/node_modules/cssesc
npm ERR! extraneous: marksy@2.0.1 /home/travis/build/airbnb/react-dates/node_modules/marksy
npm ERR! extraneous: webpack-hot-middleware@2.18.2 /home/travis/build/airbnb/react-dates/node_modules/webpack-hot-middleware
The command "if [ -n "${REACT-}" ] && [ "${TEST-}" = true ]; then sh install-relevant-react.sh && npm ls >/dev/null; fi" failed and exited with 1 during .

Do you have any idea what might be going on with that? Do we need to explicitly remove those as well? That seems... weird.

ljharb · 2017-07-25T01:05:02Z

I think the short term fix might be to temporarily remove the npm ls check in master, and then rebase this PR. I'll take care of that tomorrow.

coveralls · 2017-08-03T07:55:29Z

Coverage increased (+0.08%) to 86.324% when pulling 10def22 on mariepw:CustomPositionIcon into d32e9b3 on airbnb:master.

mariepw · 2017-08-03T07:58:09Z

Everything should be ok now ?

mariepw · 2017-08-09T15:49:41Z

Hi,
Did you have a chance to have a look on this ?
Thanks

majapw

Couple of comments! Sorry for the delay in reviewing!

majapw · 2017-08-09T16:28:53Z

  screenReaderInputMessage: '',
  showClearDates: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


Now that we have the constants file, it'd be better to import that and use BEFORE_POSITION here.

majapw · 2017-08-09T16:29:19Z

  VERTICAL_ORIENTATION: 'vertical',
  VERTICAL_SCROLLABLE: 'verticalScrollable',

+  BEFORE_POSITION: 'before',


Can we make this even more specific and call it ICON_BEFORE_POSITION or something. even ICON_BEFORE would be fine.

majapw · 2017-08-09T16:33:58Z

  readOnly: false,
  showCaret: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


Same. Let's use the variable here.

majapw · 2017-08-09T16:34:19Z


    this.onClearDatesMouseEnter = this.onClearDatesMouseEnter.bind(this);
    this.onClearDatesMouseLeave = this.onClearDatesMouseLeave.bind(this);
+    this.renderInputIcon = this.renderInputIcon.bind(this);


This doesn't need to be bound.

majapw · 2017-08-09T16:35:01Z

    });
  }

+  renderInputIcon(calendarIcon) {


I'd rather have this as a variable inputIcon in the render method than its own method.

Agreed: renderFoo methods are generally an antipattern; could this be an SFC, or inline jsx, instead?

majapw · 2017-08-09T16:35:49Z

  showCaret: false,
  showClearDate: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


majapw · 2017-08-09T16:35:53Z

  screenReaderInputMessage: '',
  showClearDate: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


majapw · 2017-08-09T16:36:09Z


    this.onClearDateMouseEnter = this.onClearDateMouseEnter.bind(this);
    this.onClearDateMouseLeave = this.onClearDateMouseLeave.bind(this);
+    this.renderInputIcon = this.renderInputIcon.bind(this);


doesn't need to be bound

majapw · 2017-08-09T16:36:18Z

    });
  }

+  renderInputIcon(calendarIcon) {


I'd prefer this as a variable in the render method

mariepw · 2017-08-10T14:36:45Z

Done :)

…specific

coveralls · 2017-08-10T14:46:41Z

Coverage increased (+0.2%) to 86.492% when pulling e572cb4 on mariepw:CustomPositionIcon into d32e9b3 on airbnb:master.

majapw

A few more comments! We're almost there, I promise

majapw · 2017-08-11T00:09:39Z

  VERTICAL_SCROLLABLE: 'verticalScrollable',

+  ICON_BEFORE_POSITION: 'before',
+  AFTER_POSITION: 'after',


sorry and can we make this one ICON_AFTER_POSITION as well?

majapw · 2017-08-11T00:10:12Z

-          </button>
-        )}
+
+        { inputIconPosition === ICON_BEFORE_POSITION && inputIcon }


nit: no spaces after { or before }

majapw · 2017-08-11T04:48:04Z

          </button>
        )}
+
+        { inputIconPosition === 'after' && inputIcon }


nit: no spaces after { or before }

Also let's use ICON_AFTER_POSITION here.

majapw · 2017-08-11T04:48:41Z

-          </button>
-        )}
+
+        { inputIconPosition === ICON_BEFORE_POSITION && inputIcon }


nit: no spaces after { or before }

majapw · 2017-08-11T04:48:51Z

          </button>
        )}
+
+        { inputIconPosition === 'after' && inputIcon }


nit: no spaces after { or before }

Also let's use ICON_AFTER_POSITION here.

Shouldn't the spaces around brackets be linted..?

ideally, but the linter is never the entirety of a codebase's style.

majapw · 2017-08-11T04:49:05Z

+
+import {
+  ICON_BEFORE_POSITION,
+  AFTER_POSITION,


ICON_AFTER_POSITION

…ant + Use constant instead of string

coveralls · 2017-08-14T05:58:43Z

Coverage increased (+0.2%) to 86.492% when pulling 2195730 on mariepw:CustomPositionIcon into d32e9b3 on airbnb:master.

mariepw · 2017-08-14T06:00:46Z

Sorry for my oversight, I've done the changes..

nikojohnarellano · 2017-12-28T00:58:47Z

I'm just wondering if this has been implemented already. If so, how can this be used as this is not part of the latest docs. Thanks

ljharb · 2017-12-28T02:30:16Z

Could you file a new issue about the lack of docs?

mariepw added 2 commits July 17, 2017 16:35

Add props to display calendar icon on right side

0a2d7eb

Fix #625 - improve code

3430bdf

Fix #625 - Fix linting

3a13698

ljharb requested changes Jul 18, 2017

View reviewed changes

Fix requested changes. Fixes #625

dc12667

ljharb previously requested changes Jul 19, 2017

View reviewed changes

mariepw added 2 commits July 19, 2017 11:09

Fix padding spaces in curly braces

8376028

Fix build failed

8a62d98

Fix warning in console 'Invalid argument supplied to oneOf, expected …

05a0d96

…an instance of array'

majapw requested changes Jul 19, 2017

View reviewed changes

Fix #625 - Extract code into reusable variables - create shape for ic…

f8c583c

…on position

Merge branch 'master' into CustomPositionIcon

10def22

majapw requested changes Aug 9, 2017

View reviewed changes

Fix #625 - Use constant value and remove render method which was too …

e572cb4

…specific

majapw reviewed Aug 11, 2017

View reviewed changes

Fix #625 - Rename AFTER_POSITION to ICON_AFTER_POSITION to be consist…

2195730

…ant + Use constant instead of string

majapw approved these changes Aug 15, 2017

View reviewed changes

majapw merged commit 13d6254 into react-dates:master Aug 15, 2017

mariepw deleted the CustomPositionIcon branch August 16, 2017 07:09

Ildarik mentioned this pull request Feb 22, 2018

Lack of docs #1037

Closed

Uh oh!

Conversation

mariepw commented Jul 18, 2017

Uh oh!

coveralls commented Jul 18, 2017

Uh oh!

coveralls commented Jul 18, 2017

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariepw commented Jul 19, 2017

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariepw Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 19, 2017

Uh oh!

coveralls commented Jul 19, 2017

Uh oh!

majapw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariepw commented Jul 20, 2017

Uh oh!

coveralls commented Jul 20, 2017

Uh oh!

mariepw commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majapw commented Jul 24, 2017

Uh oh!

ljharb commented Jul 25, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

mariepw commented Aug 3, 2017

Uh oh!

mariepw commented Aug 9, 2017

Uh oh!

majapw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mariepw Jul 19, 2017 •

edited

Loading

mariepw commented Jul 24, 2017 •

edited

Loading