[EuiDatePickerRange] Applying onFocus/OnBlur handlers to inner EuiDatePicker components #6136
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
💚 CLA has been signed |
|
Hey! Would love some further guidance on what else needs done to get this ready to ship. I didn't think any additional documentation was needed as this is fixing an issue rather than adding any new functionality. The explicit declaration of the |
|
Hey @anthonyhastings; thanks!
No documentation is needed and the added tests cover the case. The only remaining thing would be a changelog entry indicated the new behavior. jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6136/ |
| @@ -79,4 +79,51 @@ describe('EuiDatePickerRange', () => { | |||
|
|
|||
| expect(component).toMatchSnapshot(); | |||
| }); | |||
|
|
|||
| it('calls blur and focus handlers for date pickers while also triggering range control handlers', () => { | |||
There was a problem hiding this comment.
Thank you for the super explicit test!! Love it! Also agreed with making this one a unit test vs. a Cypress test, I think it's straightforward enough it doesn't need an E2E test.
There was a problem hiding this comment.
Apologies, what I'd meant was that this repository currently has npm scripts that trigger cypress component testing (rather than E2E testing).
I was going to write the test as a cypress component test so it ran in isolation within a real browser, but the test would've been orphaned considering all the existing tests would've still been in enzyme. I thought it better to opt for co-location.
|
Hey @thompsongl / @constancecchen; thanks for the feedback. That should be everything addressed! |
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6136/ |
There was a problem hiding this comment.
This looks great to me. Thank you for the fantastic community contribution @anthonyhastings! 🎉
|
You're very welcome; happy to help! Do you know what version this will be bundled with? |
|
@anthonyhastings Looks like |
Summary
Fixes #6128 which has a detailed description of the error and how to replicate it.
onBlurandonFocusto ensure they're not scooped up by the rest parameter and fed to containingdivelement.EuiDatePickerandEuiDatePickerRange. It's worth noting thatovergracefully handles array elements that aren't functions, and ignores them. This means we don't need to worry about any of the handlers not being defined.Note: I contemplated writing these tests in Cypress as I think ultimately it's a better, more user-centric test but the entirety of the date picker suite runs using Enzyme so I've opted for consistency here.
Checklist
Added documentationUpdated the Figma library counterpart