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

Support "disabled" links #926

Merged
merged 13 commits into from
May 18, 2022
Merged

Support "disabled" links #926

merged 13 commits into from
May 18, 2022

Conversation

tesk9
Copy link
Contributor

@tesk9 tesk9 commented May 16, 2022

Adds "disabled" link support by following the recommendations in Scott O'Hara's "Disabling a link" post.

Essentially, when the link is disabled:

  • removes the href
  • adds role link
  • adds aria-disabled true

Caveats

The styleguide example will NOT work correctly because of elm/browser#34, which describes a problem where "a tags without href generate a navigation event".

However, I still expect this to work in the NoRedInk context in most cases, including the case that's the reason that we're adding "disabled" link support at all.

@tesk9 tesk9 marked this pull request as ready for review May 16, 2022 23:19
@tesk9 tesk9 requested review from a team and omnibs and removed request for a team May 17, 2022 16:07
Copy link
Member

@omnibs omnibs left a comment

Choose a reason for hiding this comment

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

The changes look good. The only request I'd have before a merge is, if you're game, adding the caveat and link to the elm/browser issue in the disabled function docs, so it's easy to find if anyone gets confused in the future.

@tesk9
Copy link
Contributor Author

tesk9 commented May 18, 2022

Great idea! Doing it! Thanks

@tesk9 tesk9 merged commit 41e2c0b into master May 18, 2022
@tesk9 tesk9 deleted the bat/disabled-links branch May 18, 2022 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants