feat(DatePicker, DatePickerInput): refactors to functional component #10224
Conversation
|
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: fa9429a 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61e1f598e650020007e1bd89 😎 Browse the preview: https://deploy-preview-10224--carbon-react-next.netlify.app |
|
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: fa9429a 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61e1f5989de68300078177e6 😎 Browse the preview: https://deploy-preview-10224--carbon-components-react.netlify.app/ |
|
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: fa9429a 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61e1f598eb25010008cdbb26 😎 Browse the preview: https://deploy-preview-10224--carbon-elements.netlify.app |
packages/react/src/components/DatePickerInput/next/DatePickerInput.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/DatePickerInput/next/DatePickerInput.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/DatePickerInput/next/DatePickerInput.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think we'll need to make an update in DatePicker to make sure the openCalendar functionality works.
If I remember correctly from our work @jnm2377, the difference is in the assign* helpers in the class that assign the refs for inputField and inputToField. Let me know if I'm misremembering 🤔
…nput.js Co-authored-by: Taylor Jones <[email protected]>
@joshblack I opened the PR based on your suggestion to try to get |
jnm2377
changed the title
packages/react/src/components/DatePicker/next/DatePicker-story.js
Outdated
Show resolved
Hide resolved
|
@joshblack this is ready for re-review. I updated as best I could some of the tests we went over yesterday, but ended up simplifying a bit just for the sake of getting this in. 😅 |
packages/react/src/components/DatePicker/next/DatePicker-test.js
Outdated
Show resolved
Hide resolved
| useEffect(() => { | ||
| if (calendarRef.current) { | ||
| calendarRef.current.set({ value }); | ||
| updateClassNames(calendarRef.current); |
There was a problem hiding this comment.
Is updateClassNames something that can be pulled out of the component or does it change based on data?
There was a problem hiding this comment.
@joshblack If I understand correctly, it changes based on data. It's used when the component mounts and also here.
There was a problem hiding this comment.
@jnm2377 to avoid the react-hooks/exhaustive-deps warnings (and prevent future bugs) we should pull out updateClassNames of the component and add any additional arguments that it needs / depends on in the scope.
This should remove the need to get rid of the react-hooks/exhaustive-deps warnings (at least in cases where it's because of updateClassNames
There was a problem hiding this comment.
@joshblack Is this a blocker for this PR? I looked into it yesterday afternoon, but was running into some issues bc it's used in the useEffect that creates the flatpickr calendar, and then it's used to actually create the flatpickr calendar instance, so I'm not sure how that would work w/ pulling out the function. If it's working as expected, was wondering if this is ok as is?
There was a problem hiding this comment.
@jnm2377 the bug that we can run into is that if updateClassNames in the future is updated to use some kind of state then that state value would be stale (since the useEffect is only seeing the first "version" of this function).
What error were you seeing when you moved updateClassNames into the outermost scope of the page?
There was a problem hiding this comment.
@joshblack when I pull it out, it no longer creates the flatpickr calendar instance (from the initial useEffect), so then there are a bunch of additional errors from other parts of the code that reference the instance. I think I understand the potential issue you're describing. I guess, I feel like that's more of an enhancement to support a state.
There was a problem hiding this comment.
@jnm2377 I think what might be happening is that updateClassNames requires prefix (it's using it from the parent scope) and so when it's moved to the top-level scope things are erroring out, included the calendar creation that you're talking about.
If it is updated to include prefix as an argument I think you won't see that issue anymore, let me know!
For background on why this is something I noticed, in general, we do not want to disable the eslint rule for the rules of hooks. There are specific moments where that is okay (if we can prove the dependencies will always be up-to-date or missing deps are stable) but they should be isolated since ignoring the rules of hooks will lead to hard-to-find bugs later down the line (in my opinion)
There was a problem hiding this comment.
@joshblack I had included prefix differently (had also pulled it out to be used in the function), but hadn't tried including it as an argument. 👍🏽
There was a problem hiding this comment.
@joshblack pushed the changes, ready for review
Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Josh Black <[email protected]>
Closes #10010
Copied over from this draft: #10156
Changelog
Testing / Reviewing