-
-
Notifications
You must be signed in to change notification settings - Fork 639
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
Add link-type styling recommendation to anchor-is-valid #486
Add link-type styling recommendation to anchor-is-valid #486
Conversation
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.
Thanks - just a few comments.
|
||
const noHrefErrorMessage = 'The href attribute is required on an anchor. Provide a valid, navigable address as the href value.'; | ||
const noHrefErrorMessage = 'The href attribute is required for the link to be keyboard accessible. Provide a valid, navigable address as the href value. Alternatively, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md'; |
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.
It's not really a link unless it's an <a>
(an anchor) with an href; "on an anchor" is correct here. What about that is unclear?
Separately, if there's an href it should only ever be an <a>
- the instructions should be to help people style the <a>
as a button, not to use the wrong tag.
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.
Yeah that was not intentional. Will change the wording back to anchor.
|
||
const invalidHrefErrorMessage = 'The href attribute requires a valid address. Provide a valid, navigable address as the href value.'; | ||
const invalidHrefErrorMessage = 'The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. Alternatively, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md'; |
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.
maybe instead of "alternatively", say "if you need something that looks like a link but is not one, use a button…"?
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.
I have added similar text to that. I believe it also solves the second part of the previous comment.
Again change the element to a `<button>`: | ||
|
||
```jsx | ||
<button |
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.
all <button>
elements should have an explicit type
; let's be sure our examples do too. In this case, type="button"
(or else it'll be a submit button).
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.
100%
docs/rules/anchor-is-valid.md
Outdated
|
||
This button element can now also be used inline in text. | ||
|
||
Once again we stress that this is an inferior implementation and some users will encounter difficulty to use your website, however, it will allow a larger group of people to interact with your website than the alernative. |
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.
"than the alternative of ignoring the rule's warning"?
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.
Changed.
eb5a688
to
ed6ea93
Compare
I have addressed all the review comments aside from the comment of CSS vs inline styles. Let me know what you think. |
Can we get this in ASAP? Since this warning is showing up in CRA 2.0 apps, it's important that people understand what it's about before they disable it. This PR helps explain it. |
|
||
const invalidHrefErrorMessage = 'The href attribute requires a valid address. Provide a valid, navigable address as the href value.'; | ||
const invalidHrefErrorMessage = 'The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. If you cannot provide a valid href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md'; |
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.
Could we run these links through a url shortener? 😄 i.e. https://goo.gl/s89kbn
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.
what's the benefit of that, versus the loss of clarity on where it will redirect to?
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.
It'd reduce console noise for users who run into this a lot but know how to handle it.
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.
I can do this, if required :-)
@ljharb Do you think shortening the link will cause enough clarity loss not to cut the verbosity?
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.
We could use an aliased shortlink for improved clarity, e.g. https://tinyurl.com/anchor-is-valid
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.
Hey everyone, is this a will or won't do. I am still more than happy to do the PR. We could use Git.io
but then the url will no longer be readable at all.
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.
I’m still leaning towards “no”
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.
@Timer, as you raised it, is this something you can live with in CRA? Would be awesome to keep this rule alive in there 😄
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.
Yeah, we'll probably just replace the link ourselves in the output formatter. Thanks!
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.
Awesome!
As discussed in #485, developers face some situations where it is impossible to change design to the visual appearance of a button, or simply get faced with this rule's message and have no idea how to proceed.
This PR addx a section to the documentation of the rule explaining how a link can be changed into a button and restyled as a link in extreme circumstances.
It also adds clearer error massages.
Summary of changes:
PreferButton message:
Anchor used as a button. Anchors are primarily expected to navigate. Use the button element instead. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md
noHrefMessage:
The href attribute is required for the link to be keyboard accessible. Provide a valid, navigable address as the href value. Alternatively, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md
invalidHrefMessage:
The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. Alternatively, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md