-
Notifications
You must be signed in to change notification settings - Fork 16
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
Card: Improve accessibility by moving overlay
slot outside of <a>
tag
#650
Conversation
…and yarn versions
…t can contain a clickable element
🦋 Changeset detectedLatest commit: 00fdddc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
@@ -147,5 +139,5 @@ | |||
"path": "packages/pharos/lib/index.js" | |||
} | |||
], | |||
"packageManager": "yarn@1.22.19" | |||
"packageManager": "yarn@4.0.2" |
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.
This isn't right
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.
Let's see if we can revert all the Yarn-related items for the moment.
@@ -390,6 +390,53 @@ export const WithActionButtonSlot = { | |||
`, | |||
}; | |||
|
|||
export const WithOverlayButtonSlot = { |
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 you make sure to duplicate this in the React stories as well?
@michael-iden Wondering what the status of this is, I see it has been open a few months. Is there work needed here? Should it be closed as no longer needed? |
@brentswisher this is still an issue but was unfortunately deprioritized and I haven't gotten back around to updating things. I'll try to get an estimate on when I might be able to circle back on this fix and I'll send you a thread directly where we were tangentially talking about this problem. |
@michael-iden No worries at all, appreciate the work in this, just trying to do a little GitHub cleanup where I can! 😄 |
This change: (check at least one)
Is this a breaking change? (check one)
Is the: (complete all)
What does this change address?
Fixes a problem where the button inside of the
overlay
slot would not be picked up by the browser when tabbing through clickable elements. I believe this is because the button within theoverlay
was slotted inside of ananchor
tagHow does this change work?
Move overlay slot outside of the link tag
Additional context
I am very nervous about this change. This component is far too complex and takes in too many variable elements through slots. Even with tests and the sandbox I have little confidence that it's not actually going to break the UX in at least some browsers.
Also I am struggling to get the hooks to run properly with the shimmed yarn version. My global yarn in
4.0.1
in the node envs 18+ managed byasdf
. I might look to update the application setup to a later yarn if we can get a moment (#635).