-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix Form onBlur validation with drop components #6566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Form onBlur validation with drop components #6566
Conversation
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.
just one small comment on naming, otherwise looks good
Co-authored-by: Taylor Seamans <taylor.seamans@yahoo.com>
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.
LGTM!
Converting this PR to a draft, I realized this solution isn't working well when a Form is contained within a Layer due to the |
const dropPortalId = withinDropPortal(event.relatedTarget); | ||
if ( | ||
(dropPortalId === undefined || | ||
portalContext.indexOf(parseInt(dropPortalId, 10)) !== -1) && |
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.
Can we add a small comment speaking to what portalContext is doing here?
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.
Yes, some code moved around after I incorporated Eric's feedback so I ended up adding a comment in the utils/dom.js file to explain this
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.
Definitely improving!
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.
Thanks for moving more to DOM.js, I think it makes the component code read easier.
What does this PR do?
Fixes an issue where onBlur validation was running prematurely on components with drops (Select, SelectMultiple, DateInput) when the drop is opened.
Where should the reviewer start?
What testing has been done on this PR?
Tested with storybook story and added a jest test
How should this be manually tested?
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #5538
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Backwards compatible