Custom Input Icon--calendar position #627

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

src/components/DateRangePicker.jsx

@@ -51,6 +51,7 @@ const defaultProps = {
  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

src/components/DateRangePickerInput.jsx

@@ -123,6 +126,18 @@ export default class DateRangePickerInput extends React.Component {
    });
  }

+  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

src/components/SingleDatePickerInput.jsx

@@ -89,6 +92,18 @@ export default class SingleDatePickerInput extends React.Component {
    });
  }

+  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

src/components/DateRangePickerInput.jsx

@@ -45,6 +45,7 @@ const propTypes = forbidExtraProps({
  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

src/components/DateRangePickerInput.jsx

@@ -123,6 +126,22 @@ export default class DateRangePickerInput extends React.Component {
    });
  }

+  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

src/components/SingleDatePickerInput.jsx

@@ -89,6 +92,22 @@ export default class SingleDatePickerInput extends React.Component {
    });
  }

+  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

src/components/DateRangePicker.jsx

@@ -51,6 +51,7 @@ const defaultProps = {
  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

src/components/DateRangePickerInput.jsx

@@ -45,6 +45,7 @@ const propTypes = forbidExtraProps({
  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

src/components/DateRangePickerInput.jsx

-            {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

src/components/SingleDatePickerInput.jsx

-          >
-            {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

src/components/DateRangePicker.jsx

@@ -51,6 +51,7 @@ const defaultProps = {
  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

constants.js

@@ -10,6 +10,9 @@ module.exports = {
  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

src/components/DateRangePickerInput.jsx

@@ -87,6 +90,7 @@ const defaultProps = {
  readOnly: false,
  showCaret: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


Same. Let's use the variable here.

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

src/components/DateRangePickerInput.jsx

@@ -109,6 +113,7 @@ export default class DateRangePickerInput extends React.Component {

    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

src/components/DateRangePickerInput.jsx

@@ -123,6 +128,22 @@ export default class DateRangePickerInput extends React.Component {
    });
  }

+  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

src/components/SingleDatePickerInput.jsx

@@ -51,6 +54,7 @@ const defaultProps = {
  showCaret: false,
  showClearDate: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


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

src/components/SingleDatePicker.jsx

@@ -47,6 +47,7 @@ const defaultProps = {
  screenReaderInputMessage: '',
  showClearDate: false,
  showDefaultInputIcon: false,
+  inputIconPosition: 'before',


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

src/components/SingleDatePickerInput.jsx

@@ -75,6 +79,7 @@ export default class SingleDatePickerInput extends React.Component {

    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

src/components/SingleDatePickerInput.jsx

@@ -89,6 +94,22 @@ export default class SingleDatePickerInput extends React.Component {
    });
  }

+  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

constants.js

@@ -10,6 +10,9 @@ module.exports = {
  VERTICAL_ORIENTATION: 'vertical',
  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

src/components/DateRangePickerInput.jsx

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


nit: no spaces after { or before }

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

src/components/DateRangePickerInput.jsx

@@ -249,6 +256,9 @@ export default class DateRangePickerInput extends React.Component {
            </div>
          </button>
        )}
+
+        { inputIconPosition === 'after' && inputIcon }


nit: no spaces after { or before }

Also let's use ICON_AFTER_POSITION here.

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

src/components/SingleDatePickerInput.jsx

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


nit: no spaces after { or before }

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

src/components/SingleDatePickerInput.jsx

@@ -174,6 +184,9 @@ export default class SingleDatePickerInput extends React.Component {
            </div>
          </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

src/shapes/IconPositionShape.js

+
+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

Custom Input Icon--calendar position #627

Custom Input Icon--calendar position #627

Conversation

mariepw commented Jul 18, 2017

coveralls commented Jul 18, 2017

coveralls commented Jul 18, 2017

ljharb 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

mariepw commented Jul 19, 2017

ljharb 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

mariepw Jul 19, 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

coveralls commented Jul 19, 2017

coveralls commented Jul 19, 2017

majapw 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

mariepw commented Jul 20, 2017

coveralls commented Jul 20, 2017

mariepw commented Jul 24, 2017 • edited Loading

majapw commented Jul 24, 2017

ljharb commented Jul 25, 2017

coveralls commented Aug 3, 2017

mariepw commented Aug 3, 2017

mariepw commented Aug 9, 2017

majapw 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariepw commented Aug 10, 2017

coveralls commented Aug 10, 2017

majapw 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

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017

mariepw commented Aug 14, 2017

nikojohnarellano commented Dec 28, 2017

ljharb commented Dec 28, 2017

mariepw Jul 19, 2017 •

edited

Loading

mariepw commented Jul 24, 2017 •

edited

Loading