Skip to content

Conversation

lukaw3d
Copy link
Contributor

@lukaw3d lukaw3d commented Mar 26, 2024

Prevents unintentionally matching data and meta elements, and possibly custom web-components.

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

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)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Not certain, but I doubt it affects any real-world usecase

@@ -157,7 +157,7 @@ export const makeNodeFocusable = (node) => {
}
};

const autoFocusingTags = /(a|area|input|select|textarea|button|iframe)$/;
const autoFocusingTags = /^(a|area|input|select|textarea|button|iframe)$/;
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 comment? just for future reference
maybe something like to avoid unintentionally matching <data and <meta tags we need the ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, though it's just essential regex knowledge

Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you just good for some of us who would need to look this up and not known :)

Prevents unintentionally matching `data` and `meta` elements, and possibly
custom web-components.
@britt6612 britt6612 requested a review from taysea March 26, 2024 16:38
@taysea taysea requested a review from MikeKingdom March 27, 2024 15:59
Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

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.

4 participants