Migrated to new lifecycle method for datepiceker #10309
Conversation
Rohanhacker
changed the title
Rohanhacker
changed the title
|
Deploy preview for ant-design ready! Built with commit 1b76799 |
|
@yesmeck ok Will update |
Codecov Report
@@ Coverage Diff @@
## master #10309 +/- ##
==========================================
- Coverage 91.76% 91.75% -0.02%
==========================================
Files 199 199
Lines 5005 5011 +6
Branches 1402 1402
==========================================
+ Hits 4593 4598 +5
- Misses 407 408 +1
Partials 5 5
Continue to review full report at Codecov.
|
|
@Rohanhacker can you resolve conflict? Thanks! |
|
sure |
| if ('open' in nextProps) { | ||
| state = { | ||
| ...state, | ||
| open: nextProps.open, |
There was a problem hiding this comment.
gDSFP is not a drop in replacement for cWRP, this implementation will reset state.open on every parent rerender under React 16.4.
There was a problem hiding this comment.
@whtsky thanks, I didn't know. I'll add condition to check whether 'value' & 'open' is changed or not
| showDate: getShowDateFromValue(value) || prevState.showDate, | ||
| }; | ||
| } | ||
| if (('open' in nextProps) && prevState.open !== nextProps.open) { |
There was a problem hiding this comment.
this doesn't solve the problem: state.open still got reset to props.open whenever getDerivedStateFromProps is called.
One possible solution is to store previous props.open & props.value in state and compare them in gDSFP, then update state if they didn't match with nextProps
There was a problem hiding this comment.
@whtsky that's what I am doing no ? previous values of 'open' and 'value' are stored in state and I am comparing them in gDSFP ??
There was a problem hiding this comment.
oh, sorry. Didn’t realize value & open won’t be modified if passed in props.
| static defaultProps = { | ||
| prefixCls: 'ant-calendar', | ||
| allowClear: true, | ||
| showToday: false, | ||
| }; | ||
|
|
||
| static getDerivedStateFromProps(nextProps: any, prevState: any) { | ||
| let state = null; | ||
| if (('value' in nextProps) && !isEqual(nextProps.value,prevState.value)) { |
There was a problem hiding this comment.
Why need to compare nextProps.value and prevState.value?
There was a problem hiding this comment.
Is it ok to reset value of value on every time the parent rerenders? Like we did in componentWillReceiveProps?
There was a problem hiding this comment.
yup we did reset it every time in componentWillReceiveProps so I suppose its okay to do so ?
Ok I'll change it
|
CI failed. |
|
@yesmeck done but it seems like there is some typing error. Should do I have to update @types/react for fixing this? |
|
@Rohanhacker Yes, please. |
|
@yesmeck the lint is failing on upstream/master. Can you please check |
|
It's passing now. |
|
@yesmeck Can you review and merge ? All tests are passing now |
|
@Rohanhacker Checking. |
package.json
Outdated
| "@types/react": "^16.4.6", | ||
| "@types/react-dom": "^16.0.6", | ||
| "@types/react": "^16.0.0", | ||
| "@types/react-dom": "^16.0.0", |
There was a problem hiding this comment.
some tests were failing when I upgraded to 16.4.6 so had to downgrade
…picker
First of all, thank you for your contribution! :-)
Please makes sure that these checkboxes are checked before submitting your PR, thank you!
master, feature for latest active branchfeature-x.x.npm run lintand fix those errors before submitting in order to keep consistent code style.Extra checklist:
if isBugFix :
no
elif isNewFeature :
#9792