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

fix: link stories accessibility #34

Merged
merged 2 commits into from
Jun 27, 2019
Merged

fix: link stories accessibility #34

merged 2 commits into from
Jun 27, 2019

Conversation

jsomsanith
Copy link
Contributor

@jsomsanith jsomsanith commented Jun 26, 2019

What is the problem this PR is trying to solve?
Link stories are not accessible. This is only on the stories.

  1. links with icon only don’t have text alternative
  2. color contrast is not correct
  3. last example (ListWrapper) is weird, having a button inside an anchor

What is the chosen solution to this problem?

  1. add aria-label on link and add documentation
  2. TODO
  3. TODO: change the story

@jsomsanith
Copy link
Contributor Author

@domyen could you update the colors to have a better contract, especially on button active state (blue text on blue background)

@jsomsanith jsomsanith force-pushed the jsomsanith/fix/linnk_a11y branch from fba9dfc to 717cb12 Compare June 26, 2019 21:41
@kylesuss
Copy link
Contributor

@jsomsanith Just made an issue for this comment:

last example (ListWrapper) is weird, having a button inside an anchor

#36

It won't be as simple as just fixing the story as we need that functionality, but as you noticed, it should be done differently.

@domyen
Copy link
Member

domyen commented Jun 27, 2019

@jsomsanith and I chatted about the LinkWrapper implementation (used for Gatsby/Next/React router). We agreed to punt on it for now because it's an implementation detail that gets resolved once you put this component in a framework.

@kylesuss wrote up #36 to include LinkWrapper prop natively in the Button component so we don't need this hack.

Merging this work for now and will revisit later as part of the ticket.

@domyen domyen merged commit 62dd509 into master Jun 27, 2019
@domyen domyen deleted the jsomsanith/fix/linnk_a11y branch June 27, 2019 16:23
@kylesuss
Copy link
Contributor

🚀 PR was released in v0.0.31 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants