-
Notifications
You must be signed in to change notification settings - Fork 147
Datepicker: Refactor Calendar to fully controlled component #239
Conversation
2e29817
to
be2694a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.onDatesChange = this.onDatesChange.bind( this ); | ||
this.onFocusChange = this.onFocusChange.bind( this ); | ||
this.onInputChange = this.onInputChange.bind( this ); | ||
this.getOutsideRange = this.getOutsideRange.bind( this ); | ||
} | ||
|
||
componentDidUpdate( prevProps ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great refactoring here.
* @param {Moment|null} [after] - If already designated, the after date parameter | ||
* @param {string} format - The expected date format in a user's locale | ||
* @return {Object} validatedDate - validated date oject | ||
* @param {Moment|null} validatedDate.date - A resulting Moment date object or null, if invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two @param
s needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think so. They are describing the returned object I've called validatedDate
.
|
||
it( 'should return a correct error for a date in the future', () => { | ||
const futureDateString = moment() | ||
.add( 1, 'months' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
839eb2c
to
b80968a
Compare
094e186
to
10099db
Compare
10099db
to
b7dc5d1
Compare
Thanks for taking a look @timmyc. The popover being unscrollable was fixed in #235 (comment). Those changes probably hadn't made their way to this branch yet, sorry about that. This branch is rebased and now has the fix. |
I'm still seeing a bit of oddness with the same issue I described above. Almost seems like my viewport height is at some in-between spot that is preventing the scrolling from happening. If I shrink the viewport a smidge, it works as expected. I still think we should get this merged in, and we can open up a separate issue and work on a fix there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm voting to merge this in, and do follow-up fixes as we identify any other problems, like my odd scroll one. I also noticed a z-index issue too with the master/nav bar which we can do in a follow-up.
I can now reproduce this when opening the picker and then switching to "Custom" Issue for that and z-index here #252 |
The Calendar component had its own state, comprised of text inputs and error messages. This PR moves the state logic up to the date picker, making Calendar a fully controlled component.
There was a mis-alignment between the Calendar state and DatePicker state. In particular, the Reset button had effects on both and some comparisons were required in Calendar to guess if the DatePicker state had been reset. This caused odd interactions when resetting. I can now eliminate this faulty code:
Other changes
validateDateInputForRange
was also pulled out tolib/date
and unit tests added.UNSAFE_componentWillReceiveProps
.The nature of this component is such that a reset of internal state needs to happen on every change of the url query parameters. Previously, this was done like in the
UNSAFE
manner:One option was to use
getDerivedStateFromProps
, but that runs on every update and can introduce problems. Instead I opted for Fully uncontrolled component with a key to force reset on prop changes.Test
/wp-admin/admin.php?page=wc-admin#/analytics/products
npm run test
Depends on #235