Skip to content

Conversation

@khiga8
Copy link
Collaborator

@khiga8 khiga8 commented Jun 9, 2022

Relates to: https://github.com/github/accessibility/issues/1291 (GitHub Staff-only)

We often use generic link text in our ERB files. This PR adds an ERB linter against generic link text in ERB. This linter rule will report any generic text usage within anchor tags as well as in Rails helpers like link_to which generate anchor tags.

I would appreciate extra eyes on the documentation and whether this BANNED_GENERIC_TEXT is reasonable. Any more to add to this list?

message ||= klass::MESSAGE
message += "\nLearn more at https://github.com/github/erblint-github#rules.\n"
offense = ["#{simple_class_name}:#{message}", source_range.source].join("\n")
add_offense(source_range, offense, replacement)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This helper allows the output of the ERB lint message to be something like:

ERBLint::Linters::A11yAvoidGenericLinkTextCounter:Avoid using generic link text such as Read more, Learn more, Click here, More, Link.
<a href="github.com/about">Learn more
In file: app/views/test.html.erb:46

where we can specify a range of the code source to be output. In this case we include the first anchor tag and the text content for ease of identification.

The other helper we commonly use, generate_offense only allows a single tag to be output in the message.

include ERBLint::Linters::CustomHelpers
include LinterRegistry

BANNED_GENERIC_TEXT = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there anything to remove from here? Anything else we should add in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is amazing 🤩 Thank you for doing this! What do you think about adding "Here" as well? I've seen text such as `Read more about it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea!!! <3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated in: 80ebd6b ❤️

@khiga8 khiga8 marked this pull request as ready for review June 9, 2022 15:25
@khiga8 khiga8 requested a review from a team as a code owner June 9, 2022 15:25
Copy link
Contributor

@andrialexandrou andrialexandrou left a comment

Choose a reason for hiding this comment

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

Thoughts

  1. OMG love
  2. there was a really great slack thread on link texts where a few of our designers chimed in about the complexities here, that I think are worth surfacing. the cognitive ease of a marketing page that repeats "Learn more" links at the end of bullet points was a good example of how sometimes it's not clear what the "most accessible" choice is, and it's worth providing the complicated scenarios in our guidance. "Go to plans", e.g., was the eventual solution that suited all cases, for instance.
  3. do our listing errors guide people to documentation and slack channels? Joel mentioned it and since I'm never working in dotcom I'm not sure the answer

@khiga8
Copy link
Collaborator Author

khiga8 commented Jun 10, 2022

@andrialexandrou

  1. thanks for surfacing that thread! Lots of good links and conversation in there. I'll look through it and update the documentation page!
  2. We do indeed. When the linters run in dotcom, a message gets appended of how to reach our team. (Staff only link: dotcom code appending message). In generate_offense, we also append a link to the documentation as part of the error message. :)

@khiga8
Copy link
Collaborator Author

khiga8 commented Jun 10, 2022

@andrialexandrou I updated the documentation in c077f6d! Would you mind another review?

Copy link
Contributor

@andrialexandrou andrialexandrou left a comment

Choose a reason for hiding this comment

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

Looks great 🚢

@khiga8 khiga8 merged commit e8cf7f7 into main Jun 15, 2022
@khiga8 khiga8 deleted the kh-avoid-generic-link branch June 15, 2022 19:34
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.

4 participants