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

Remove broken-link flaws for self-links #4095

Open
Tracked by #5156
hamishwillee opened this issue Jun 26, 2021 · 10 comments
Open
Tracked by #5156

Remove broken-link flaws for self-links #4095

hamishwillee opened this issue Jun 26, 2021 · 10 comments
Labels
flaw-system issues and feature requests related to the flaws system 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. idle p2 We want to address this but may have other higher priority items.

Comments

@hamishwillee
Copy link
Contributor

hamishwillee commented Jun 26, 2021

Can we please

  1. remove the flaw for links-to-self as a flaw except perhaps if the link is via a redirect
  2. Right now self links are shown as bold and black which is great - they don't look like links. But we're only half way there. Hover shows that they are still links and if you click them then you reload the page. We don't want that - a self link in the sidebar and in text should be unclickable so you never lose the current context.

TLDR below

Originally we made links-to-self a flaw, because a doc should not link to itself. The case that triggered this was an originally valid link where the target page was replaced by a redirect back to the current page.

But there are a bunch of intentional links to self - for example:

  • learn module topics in a set all have in-page navigation (a list of topics in the module), one of which will be the current page. - It is not uncommon for a page to use an xref macro for marking up the current reference item - which creates a link to self.
  • Sidebar has link to the current page - that is needed to provide context but should not be clickable because that loses your context.

We fixed these by making the links auto-render as black bold text. Works great - rather than having to manually make all the links into just text this happens by magic.

However this fix is not quite complete - the links are still links when you press them. It would be better if they were not clickable - they are simply not rendered as links.

Further, if they are not links then there is no flaw, and we should not report a flaw in most cases. The exception is the case where the self link happens due to a redirect. That essentially means that the link used to go to something valid and does not any more.

I guess we could keep it a flaw in all cases and change the rendering back to a link, but then we'd want to fix every single case in the docs. For the learn section we'd want to decide a better way to do the sidebar.

@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Dec 8, 2021
@hamishwillee
Copy link
Contributor Author

@schalkneethling Can this go on your list for consideration.

  • People fix the flaws because they are reported: Update links content#11643 - you can see the case in here
  • The link should not be clickable because it takes people back to same page and loses their current context. This is also true in sidebar for the self link to current page.

image

@github-actions github-actions bot removed the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Jan 2, 2022
@schalkneethling
Copy link

@hamishwillee Thank you for the ping. I will add this to my list.

@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Feb 10, 2022
@hamishwillee
Copy link
Contributor Author

Yeah, this shouldn't be closed.

@teoli2003
Copy link
Contributor

I would like to confirm that this issue has annoying effects:

  • Editors sometimes deleted such links in lists, and these lists in the LA are part of the navigation process.
  • They fill the flaw dashboard about broken links/redirect links with a few hundreds of bogus reports, making it hard (and time-consuming) to find actually broken links.
  • Maintaining such lists for the Learning Area is next to impossible. When doing a change, you have to manually edit each page instead of cut/pasting the new block (not ideally, but far less error-prone)

Note that we still want to report links to the same page that have an anchor (we want to keep only the anchor, not the file path): we want to fix these.

This would be in my top-5 issues to fix.

@schalkneethling
Copy link

Thank you, @teoli2003! I am going to turn this into a discussion and bring it to the attention of engineering. I know there is a lot of work happening at the moment to refactor and improve Yari overall, so it would be good to have this input.

@schalkneethling
Copy link

@hamishwillee
Copy link
Contributor Author

@schalkneethling FWIW without wanting to distract from this conversation ....

The problem with Learn module navigation links like this one: https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Client-side_web_APIs/Client-side_storage#in_this_module is actually different. Yes the bug shows up, but the root problem for these is that they should be in the sidebar rather than the page.

Unfortunately they can't be because the sidebar is broken.
In a working sidebar (a) the currently selected page should be highlighted, and (b) when you switch to the page the sidebar should scroll that page into view, (c) usually also collapsing the rest of the sidebar to open just that nested level.

Here is my proof: https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/Document_and_website_structure - where is the document in the sidebar? If you have to scroll and there is nothing to catch the eye then you have lost context. The site is broken.

Note, (a) has been fixed several times, last time since the rebrand, but is broken again:
image

If I sound frustrated it's because I am. IMO this should have been done as a higher priority than the website refresh.

@github-actions github-actions bot removed the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label May 31, 2022
@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Jul 7, 2022
@caugner
Copy link
Contributor

caugner commented Aug 18, 2022

Can we please

1. remove the flaw for links-to-self _as a flaw_ except perhaps if the link is via a redirect

2. Right now self links are shown as bold and black which is great - they don't look like links. But we're only half way there. Hover shows that they are still links and if you click them then you reload the page. We don't want that - a self link in the sidebar and in text should be unclickable so you never lose the current context.
  • To solve 1., we could simply remove these lines:

    } else {
    addBrokenLink(
    a,
    checked.get(href),
    href,
    null,
    "Link points to the page it's already on",
    null,
    true
    );
    }

  • To solve 2., we could loop over all links here, and replace exact self-links (no anchor) by a <strong> tag:

    yari/build/index.js

    Lines 511 to 515 in a883a80

    // All external hyperlinks should have the `external` class name.
    postProcessExternalLinks($);
    // All internal hyperlinks to a file should become "absolute" URLs
    postLocalFileLinks($, doc);

IMO we should implement 1., but not necessarily implement 2., because it adds complexity to our document build, which we're actually trying to simplify.

@caugner
Copy link
Contributor

caugner commented Aug 18, 2022

@hamishwillee I would suggest renaming this issue to "Remove broken-link flaws for self-links", and then we can finally tackle it.

@caugner caugner added flaw-system issues and feature requests related to the flaws system and removed 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. needs decision from engineering labels Aug 18, 2022
@hamishwillee hamishwillee changed the title Improve self link broken link flaw checking Remove broken-link flaws for self-links Aug 19, 2022
@hamishwillee
Copy link
Contributor Author

hamishwillee commented Aug 19, 2022

@caugner So you're saying this will just fix the flaws on self links, but not make the self-link non-clickable? Either way, I'm OK with this because we need to get the flaw fixed first.

Note that a self link via a redirect would ideally still remain a flaw.

Renamed now!

@caugner caugner added the p2 We want to address this but may have other higher priority items. label Sep 12, 2022
@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Oct 19, 2022
@github-actions github-actions bot added the idle label Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaw-system issues and feature requests related to the flaws system 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. idle p2 We want to address this but may have other higher priority items.
Projects
Development

No branches or pull requests

4 participants