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

Issue with all content: Missing link underlines #2901

Closed
aardrian opened this issue Feb 11, 2021 · 50 comments · Fixed by #3171
Closed

Issue with all content: Missing link underlines #2901

aardrian opened this issue Feb 11, 2021 · 50 comments · Fixed by #3171
Assignees
Labels
♿ a11y An MDN Web Docs accessible by everyone p1 We will address this soon and will provide capacity from our team for it in the next few releases.

Comments

@aardrian
Copy link

MDN URL: https://developer.mozilla.org/en-US/docs/ (and all pages below)

Issue

None of the links within the content have an underline. They are difficult to distinguish from the surrounding content and require interaction to identify them in some cases.

The page content is dark gray (#212121) and the link color is dark blue (#00458b). with a contrast ratio of 1.7:1, it is fair to assume most users with good vision under ideal conditions will have to take a second look.

As it stands now, the content links would fail WCAG 2.1 Success Criterion 1.4.1 Use of Color. The non-normative G182 recommends using an additional visual cue besides color. G183 recommends a 3:1 contrast ratio and an additional visual cue on focus or hover, but is now mooted with the advent of touch interfaces. I go into more detail about alternatives to underlines in On Link Underlines.

Recommended fix

To restore underlines to links in the content…

In the file:
/node_modules/@mdn/minimalist/sass/atoms/_typography.scss

At line 69, please strike text-decoration: none;. Reference code with - preceding the line to strike:

  /* text links */
  a:link,
  a:visited {
    color: $link-color;
-    text-decoration: none;

    &:hover,
    &:focus,
    &:active {
      text-decoration: underline;
    }
  }

In the file:
/node_modules/@mdn/minimalist/sass/atoms/_links.scss

At line 3, please strike text-decoration: none;. Reference code with - preceding the line to strike:

  a {
    color: $link-color;
-    text-decoration: none;

    &:hover,
    &:focus {
      text-decoration: underline;
    }

This will restore underlines to links in the navigation and elsewhere as well. If you want to remove those, I suggest more targeted removal of underlines scoped to just those areas to ensure underlines are retained on links outside the content.

If you would like to try the content with restored underlines without fiddling in the browser's dev tools, I have bookmarklet that will restore underlines (which I tested on MDN) at http://rosel.li/bookmarklets#underlines.

Specific section or headline?

Throughout.

What did you expect to see?

Underlines on links.

Did you test this? If so, how?

I tried to use the content.

MDN Content page report details
@chrisdavidmills
Copy link
Contributor

@aardrian , you are a saint ;-)

I have tried to get link underlines added to MDN a couple of times, and it seems good to revisit this.

The person to talk to about this would be @schalkneethling , our front-end dev. What do you think man? What is the usual procedure for making such sweeping changes, and where would be best to file this issue?

@Ryuno-Ki
Copy link

I'd say, this issue should be transferred from mdn/content to mdn/mdn-minimalist, since it's a styling issue.
(Do we need to respect mozilla.design branding guidelines?)

@dhananjayd1729
Copy link

Hello @chrisdavidmills and @aardrian, since this is an easy fix, I would like to work on this. This would be my first bug fix. Can I start working on the same?

@aardrian
Copy link
Author

aardrian commented Feb 11, 2021

@dhananjayd1729 I have no say in whether you can jump on this.

However, as I note in the issue:

This will restore underlines to links in the navigation and elsewhere as well. If you want to remove those, I suggest more targeted removal of underlines scoped to just those areas to ensure underlines are retained on links outside the content.

So there is a risk your PR will be rejected if it enables underlines in navigation, footer, etc.

@simevidas
Copy link

A quick reminder that underlines can also be styled now, e.g.,

a {
  text-underline-offset: .13em;
  text-decoration-thickness: .01em;
}

@dhananjayd1729
Copy link

@aardrian I will try to underline only those links which are actually in the body of the HTML page(i.e apart from the footer and navigation). Will that work?

@Ryuno-Ki
Copy link

To avoid duplicated efforts on this, I assigned this issue to @dhananjayd1729 in the meantime.

@dhananjayd1729
Copy link

Thank you, @Ryuno-Ki

@schalkneethling schalkneethling transferred this issue from mdn/content Feb 12, 2021
@schalkneethling schalkneethling transferred this issue from mdn/mdn-minimalist Feb 12, 2021
@schalkneethling
Copy link

Thank you @aardrian for reporting this. It is a BIG oversight. Whoever picks this up, here is what you are going to need to change:

a {
    text-decoration: underline;

    &:active {
      background-color: $primary-50;
      color: $text-color-inverted;
    }
  }

@mdn mdn deleted a comment from github-actions bot Feb 12, 2021
@peterbe
Copy link
Contributor

peterbe commented Feb 12, 2021

To avoid duplicated efforts on this, I assigned this issue to @dhananjayd1729 in the meantime.

Please don't assign issues. Not even to people in the core team. We assign them to ourselves or perhaps in some exceptions, for each other after we've had offline conversations.

Contributors are more than welcome to make pull requests but they have to take the chance that a PR might come in slightly later than another contributors PR.

All too often we've seen it happen over the years that well-meaning contributors show interest in solving an issue (and sometimes get assigned) but the PR never comes.

To us, assignment is similar to putting something on our "weekly" to-do list. It's not ownership or best-matched domain/area.

@ghost
Copy link

ghost commented Feb 12, 2021

In my opinion, it could be an anarchist attempt in which everyone can self-assign issues and even steal them from others and see if the team manages itself as the Agile theorists theorize.


they have to take the chance that a PR might come in slightly later than another contributors PR.

This Is a Issue.


Aside from that, you should investigate where the links have been unsubscribed and not add places where they are underlined (although it may seem like a maintenance gesture). And it would be nice to give a touch of style 🖌️ to the underlining perhaps with mathematically reasoned values.


Even di could change from none to inherit to be explicit that we want underline as the computed value. (🤔 Should works 🧂)

@dhananjayd1729
Copy link

I have been trying to run 'yari' on my pc but I'm getting an error. I followed the instructions given in the readme file. I'm attaching ss, please have a look. I would appreciate the help.
githuberror

@schalkneethling
Copy link

I have been trying to run 'yari' on my pc but I'm getting an error. I followed the instructions given in the readme file. I'm attaching ss, please have a look. I would appreciate the help.

Did you clone the content repo at the URL below to that location @dhananjayd1729?
https://github.com/mdn/content

@dhananjayd1729
Copy link

Yes, I did. I cloned 'content' and 'yari' repo in the 'mozilladocs' folder.

@schalkneethling
Copy link

Yes, I did. I cloned 'content' and 'yari' repo in the 'mozilladocs' folder.

Ah! Just noticed. You need to add /files/ so, /content/files/

@Ryuno-Ki
Copy link

Also, is the C:\c\ in the screenshot on purpose?
(I've got a similar case on Matrix at the moment)

@schalkneethling
Copy link

Yes, I did. I cloned 'content' and 'yari' repo in the 'mozilladocs' folder.

All good now @dhananjayd1729?

@schalkneethling schalkneethling added the ♿ a11y An MDN Web Docs accessible by everyone label Feb 16, 2021
@dhananjayd1729
Copy link

Yes, I did. I cloned 'content' and 'yari' repo in the 'mozilladocs' folder.

All good now @dhananjayd1729?

I will try in a bit and will comment here.

@dhananjayd1729
Copy link

@schalkneethling .
newerror

@schalkneethling
Copy link

@dhananjayd1729 That is interesting. I am not entirely sure on that one, I reckon either @fiji-flo or @peterbe will be able to assist here.

On a side note, are you planning on working on this issue? In other words, is that why you are getting all of this set-up? Or is it unrelated?

@dhananjayd1729
Copy link

Thanks, @schalkneethling. Yes, I'm planning to work on this issue and would like to work on more issues in the future.

@fiji-flo
Copy link
Contributor

@dhananjayd1729 what's your CONTENT_ROOT when you're running this?

Can you run yarn tool gather-git-history?

@dhananjayd1729
Copy link

@fiji-flo CONTENT_ROOT=/files . I didn't run 'yarn tool gather-git-history' yet.

@Ryuno-Ki
Copy link

On Windows, %SOMETHING% refers to an environment variable „SOMETHING”.
%UserProfile% (since Windows is case insensitive, you might have seen this notation) should point to C:\Users\daund in your case.

@dhananjayd1729
Copy link

@dhananjayd1729 what's your CONTENT_ROOT when you're running this?

Can you run yarn tool gather-git-history?

After running this command I've got this error.
yarngather1

@peterbe
Copy link
Contributor

peterbe commented Feb 16, 2021

@dhananjayd1729 Can you run that again but with this:

yarn tool gather-git-history --verbose

It won't solve the immediate problem but next time, it'll print out a log stacktrace that shows exactly in which file that happened.

@dhananjayd1729
Copy link

@dhananjayd1729 You up and running?

No, I'm stuck even after 'content root' points to files folder in a clone of 'content' .

@peterbe
Copy link
Contributor

peterbe commented Feb 19, 2021

@dhananjayd1729 You up and running?

No, I'm stuck even after 'content root' points to files folder in a clone of 'content' .

Another important fix landed today relating to Windows and that gather-git-history. Can you try with the latest main again?

@dhananjayd1729
Copy link

@dhananjayd1729 You up and running?

No, I'm stuck even after 'content root' points to files folder in a clone of 'content' .

Another important fix landed today relating to Windows and that gather-git-history. Can you try with the latest main again?

'Latest main' as in?

@peterbe
Copy link
Contributor

peterbe commented Feb 19, 2021

We used to have the default branch be called master. Now it's called main.

@dhananjayd1729
Copy link

So you are basically suggesting me to change the directory to content? Because yari is the master branch at this stage.

@peterbe
Copy link
Contributor

peterbe commented Feb 19, 2021

So you are basically suggesting me to change the directory to content? Because yari is the master branch at this stage.

I know it's confusing with all the branches and stuff. It takes some getting used to.
But if it's really tricky, consider starting over a bit and seeing if a fresh clone helps.

@Ryuno-Ki
Copy link

@dhananjayd1729 In #2951 @peterbe explained how to update a local setup to use main on Yari, too.

@peterbe
Copy link
Contributor

peterbe commented Mar 2, 2021

Thank you @aardrian for reporting this. It is a BIG oversight. Whoever picks this up, here is what you are going to need to change:

a {
    text-decoration: underline;

    &:active {
      background-color: $primary-50;
      color: $text-color-inverted;
    }
  }

I tried that but it only underlines the a within the articles. And the headline links too.
Other features such as the document footer, sidebars, breadcrumbs, and home page are unaffected.
Screen Shot 2021-03-02 at 9 04 22 AM

I'm too old-school to understand why minimalist even removes this. I think it should be reset/default and then we can make exceptions on top. Like: h2 a, h3 a { text-decoration:none}

@aardrian
Copy link
Author

aardrian commented Mar 2, 2021

Since the content links are the ones that need the underlines, not affecting footer, sidebars, and breadcrumbs seems fine. I agree that the headings likely do not need underlines (though I do like the pattern of an icon to indicate they can be used as anchor links for sharing, making them distinct from content links that navigate someone).

@schalkneethling
Copy link

Other features such as the document footer, sidebars, breadcrumbs, and home page are unaffected.

That is fine and the intent of the change here.

I'm too old-school to understand why minimalist even removes this.

I did consider it but, I think we will have to undo more underlining than we currently will have to add exceptions where we do want underlines.

@schalkneethling
Copy link

@dhananjayd1729 were you able to get up and running?

@dhananjayd1729
Copy link

@schalkneethling I was having exams back then so I couldn't look into it. But yeah from today onwards I will try to run again.

@schalkneethling
Copy link

@schalkneethling I was having exams back then so I couldn't look into it. But yeah from today onwards I will try to run again.

Ok, let me know if you run into problems or have any questions.

@schalkneethling
Copy link

@dhananjayd1729 This is on the critical path for us so I am going to pick this one up. Please have a look at issues marked as good first issues or just let us know if you are interested in picking something else up. I hope you have gotten Yari working locally. Thank you again for your interest in contributing.

@schalkneethling schalkneethling self-assigned this Mar 9, 2021
@schalkneethling schalkneethling added the p1 We will address this soon and will provide capacity from our team for it in the next few releases. label Mar 9, 2021
peterbe pushed a commit that referenced this issue Mar 11, 2021
* fix: links in article content

Fixes underlines and colors of links in main page content.

Fix #2901

* update print style rules

* underline links in meta document footer
@aardrian
Copy link
Author

Glad to see this wrapped up, thanks all!

How long before it appears on the site? I just purged my cache to be sure it is not already out there before asking.

@escattone
Copy link
Contributor

@aardrian It should be fully deployed about 5-6 hours from now, and then it's a matter of when each page's CDN cache expires (at most 24 hours). So, at the latest, all pages should show this change 30 hours from now. Cheers.

@dhananjayd1729
Copy link

@dhananjayd1729 This is on the critical path for us so I am going to pick this one up. Please have a look at issues marked as good first issues or just let us know if you are interested in picking something else up. I hope you have gotten Yari working locally. Thank you again for your interest in contributing.

@schalkneethling First of all I'm really sorry for being not active on time(there was a lot things going with me lately, my exams and all).But when I said I want to work on this issue that time I tried my best to run yari locally but I couldn't. I would like to contribute to MDN docs for upcoming issues.I hope I will get help when I get stucked.

@schalkneethling
Copy link

@dhananjayd1729 This is on the critical path for us so I am going to pick this one up. Please have a look at issues marked as good first issues or just let us know if you are interested in picking something else up. I hope you have gotten Yari working locally. Thank you again for your interest in contributing.

@schalkneethling First of all I'm really sorry for being not active on time(there was a lot things going with me lately, my exams and all).But when I said I want to work on this issue that time I tried my best to run yari locally but I couldn't. I would like to contribute to MDN docs for upcoming issues.I hope I will get help when I get stucked.

No need to apologize @dhananjayd1729. Please let us know whenever you are ready and find something you are interested in.

@dhananjayd1729
Copy link

Sure @schalkneethling . Looking forward to contribute.

peterbe pushed a commit to peterbe/yari that referenced this issue Jun 1, 2021
* fix: links in article content

Fixes underlines and colors of links in main page content.

Fix mdn#2901

* update print style rules

* underline links in meta document footer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♿ a11y An MDN Web Docs accessible by everyone p1 We will address this soon and will provide capacity from our team for it in the next few releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants