Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

michael-iden
Copy link
Contributor

@michael-iden michael-iden commented Nov 20, 2023

This change: (check at least one)

  • Adds a new feature
  • Fixes a bug
  • Improves maintainability
  • Improves documentation
  • Is a release activity

Is this a breaking change? (check one)

  • Yes
  • No - Although I think its pretty close to a breaking change

Is the: (complete all)

  • Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
  • Test suite(s) passing?
  • Code coverage maximal?
  • Changeset added?
  • Component status page up to date?

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 the overlay was slotted inside of an anchor tag

How 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 by asdf. I might look to update the application setup to a later yarn if we can get a moment (#635).

Copy link

changeset-bot bot commented Nov 20, 2023

🦋 Changeset detected

Latest commit: 00fdddc

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

This PR includes changesets to release 1 package
Name Type
@ithaka/pharos 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

github-actions bot commented Nov 20, 2023

size-limit report 📦

Path Size
packages/pharos/lib/index.js 52.79 KB (+0.01% 🔺)

@@ -147,5 +139,5 @@
"path": "packages/pharos/lib/index.js"
}
],
"packageManager": "yarn@1.22.19"
"packageManager": "yarn@4.0.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't right

Copy link
Member

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 = {
Copy link
Member

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?

@eslawski eslawski removed their request for review April 24, 2024 12:14
@brentswisher
Copy link
Contributor

@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?

@michael-iden
Copy link
Contributor Author

@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.

@brentswisher
Copy link
Contributor

@michael-iden No worries at all, appreciate the work in this, just trying to do a little GitHub cleanup where I can! 😄

@ymouzakis ymouzakis closed this Jun 12, 2024
@ymouzakis ymouzakis deleted the bugfix/card-accessibility branch June 12, 2024 17:46
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.

4 participants