Skip to content

Conversation

jcfilben
Copy link
Collaborator

@jcfilben jcfilben commented Jan 6, 2023

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.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • 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

@jcfilben jcfilben requested a review from taysea January 6, 2023 04:41
Copy link
Collaborator

@taysea taysea left a 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

@jcfilben jcfilben requested a review from taysea January 6, 2023 18:53
Co-authored-by: Taylor Seamans <taylor.seamans@yahoo.com>
@taysea taysea self-requested a review January 6, 2023 22:24
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@taysea taysea requested a review from ericsoderberghp January 6, 2023 22:24
@jcfilben
Copy link
Collaborator Author

jcfilben commented Jan 9, 2023

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 withinDropPortal function.

@jcfilben jcfilben marked this pull request as draft January 9, 2023 19:47
@jcfilben jcfilben marked this pull request as ready for review January 10, 2023 00:14
const dropPortalId = withinDropPortal(event.relatedTarget);
if (
(dropPortalId === undefined ||
portalContext.indexOf(parseInt(dropPortalId, 10)) !== -1) &&
Copy link
Collaborator

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?

Copy link
Collaborator Author

@jcfilben jcfilben Jan 12, 2023

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

Copy link
Contributor

@ericsoderberghp ericsoderberghp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely improving!

Copy link
Contributor

@ericsoderberghp ericsoderberghp left a 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.

@ericsoderberghp ericsoderberghp merged commit 4ff8bba into grommet:master Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown component: When it is required clicking on the dropdown itself is giving red border
3 participants