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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow Banner component to receive focus, adds optional id
parameter
#2576
base: main
Are you sure you want to change the base?
Conversation
|
This seems like a better idea to me. With an |
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
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?
Note: I am mostly looking for feedback on this approach before I test it fully 馃檹馃徎
On the Accessibility team, we've discovered a few audit issues are marked as blocked by Primer. After more investigation, we realized this is not the case, and that in some scenarios, we want to move focus to the Banner component when a user takes an action. We can do this in dotcom with JS, as most consumers have logic to show/hide the banners based on certain conditions.
This PR adds a
focusBanner()
method to allow consumers to hook into to focus the banner in certain scenarios. The element we want to focus on in a Banner is subject to change, so this approach allows that element to be managed at the Primer level. It also allows an optionalid
parameter to be passed in which is applied to the<x-banner>
element (currently passing inid
as a system arg applies it to the nested<div>
underx-banner
, so calling a method on the ID won't work).Integration
No
List the issues that this change affects.
Risk Assessment
What approach did you choose and why?
I chose to add a method to add the
tabindex
and.focus()
functionality to focus the banner. @khiga8 and I considered adding a parameter to the ruby component, such asfocus_on_show
, that would hide the banner by default, and when shown, it would focus the banner.The reasons we didn't go with this approach right now (it may be a good thing to discuss in the future!):
Anything you want to highlight for special attention from reviewers?
A consumer could use this new functionality by calling the method this way:
Is this an anti-pattern? Should we invest more time in allowing a consumer to do something like:
and then the component would be responsible for showing/hiding itself? I think we'd still need to have an event listener present to know when to show/hide the component... Open to other ideas here!
Accessibility
Allows the banner to be focusable, which will ensure screen reader announcements are consistent in certain scenarios when relevant
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.