Skip to content

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Feb 10, 2024

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

Currently, when the Banner is set to dismissable, the close button will steal focus because of the autofocus attribute.

This PR removes the autofocus attribute.

Autofocusing the close button seems like unexpected behavior... For example, if you navigate to Dismissable Banner example on Lookbook, you'll notice the close button steals focus. It seems disorienting if a user opens a page, and their focus gets stolen by a close button on a Banner that's far down the page. I am not sure why this attribute is set.

Integration

No

List the issues that this change affects.

Fixes: #2586

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

Anything you want to highlight for special attention from reviewers?

I may be lacking context on why this attribute is set, so would appreciate if I'm missing anything.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@khiga8 khiga8 requested review from a team and keithamus February 10, 2024 05:06
Copy link

changeset-bot bot commented Feb 10, 2024

🦋 Changeset detected

Latest commit: cff3e11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@khiga8 khiga8 changed the title Bug: Remove dismissable flash stealing focus Bug: Remove dismissable Banner stealing focus Feb 12, 2024
Copy link
Contributor

@lindseywild lindseywild left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me!

Copy link
Collaborator

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@camertron camertron merged commit 6124098 into main Feb 12, 2024
@camertron camertron deleted the kh-remove-autofocus branch February 12, 2024 19:56
@primer primer bot mentioned this pull request Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Dismissable Banner close button has autofocus
3 participants