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

Add link-type styling recommendation to anchor-is-valid #486

Merged

Conversation

AlmeroSteyn
Copy link
Contributor

@AlmeroSteyn AlmeroSteyn commented Oct 2, 2018

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:

  1. Add an explanatory section to the docs:

image

  1. Change the error messsage to:

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

@coveralls
Copy link

coveralls commented Oct 2, 2018

Coverage Status

Coverage remained the same at 99.618% when pulling ed6ea93 on AlmeroSteyn:anchor-is-valid-clickable-link into 71819a0 on evcohen:master.

Copy link
Member

@ljharb ljharb left a 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';
Copy link
Member

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.

Copy link
Contributor Author

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';
Copy link
Member

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…"?

Copy link
Contributor Author

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

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

Copy link
Contributor Author

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 Show resolved Hide resolved

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.
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@AlmeroSteyn AlmeroSteyn force-pushed the anchor-is-valid-clickable-link branch from eb5a688 to ed6ea93 Compare October 3, 2018 17:38
@AlmeroSteyn
Copy link
Contributor Author

I have addressed all the review comments aside from the comment of CSS vs inline styles.

Let me know what you think.

@gaearon
Copy link

gaearon commented Oct 5, 2018

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';
Copy link

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

Copy link
Member

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?

Copy link

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.

Copy link
Contributor Author

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?

Copy link

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

Copy link
Contributor Author

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.

Copy link
Member

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”

Copy link
Contributor Author

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 😄

Copy link

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome!

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.

6 participants