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

feat(v2): add a banner that links to latest version of documentation #2916

Merged
merged 21 commits into from
Jun 15, 2020

Conversation

teikjun
Copy link
Contributor

@teikjun teikjun commented Jun 10, 2020

Motivation

Add a badge that links older versions of documentation to the latest version of the documentation. This PR closes #2432.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

  1. After visiting the netlify preview site, go to any older version of the documentation . Check that there is a badge that links to the latest version
    image

  2. Click on the link to the latest version. There should be no badge next to the version number, since we are now at the latest version
    image

The automated tests has been updated accordingly.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 10, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 10, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 12ef010

https://deploy-preview-2916--docusaurus-2.netlify.app

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Hi and thanks for your PR.

isOld and link to /docs is likely not reliable enough. It will work on basic D2 sites but will not work on sites that configure routeBasePath or have no homePageId

@@ -115,6 +117,11 @@ function DocItem(props) {
<span className="badge badge--secondary">
Version: {version}
</span>
{isOld ? (
<span className="badge badge--secondary margin-horiz--xs">
<Link to="/docs"> Go to latest version </Link>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The /docs url is configurable, so it's not a good idea to hardcode the link to /docs here, + there's no guarantee that the user used the docs "homePageId" feature.

See https://v2.docusaurus.io/docs/docs-introduction/#docs-only-mode

@@ -145,6 +150,7 @@ export default async function processMetadata({
lastUpdatedBy,
lastUpdatedAt,
sidebar_label,
isOld,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rather try to compute, for each doc, what's the permalink to the latest version of the same doc.

maybe we need a property like latestPermalink

Copy link
Contributor

Choose a reason for hiding this comment

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

some docs may get deleted in newer version of doc

Copy link
Collaborator

Choose a reason for hiding this comment

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

In such case latestPermalink would be undefined, and you wouldn't add the link?

Ah that's right, you want to link to the docs root, not necessarily the same doc in another version, so this is probably overkill.

@slorber
Copy link
Collaborator

slorber commented Jun 10, 2020

Hey @teikjun

I have this other PR with some changes to metadata.ts, just wanted to mention that so that we can avoid too much conflicts :)
#2905

@slorber
Copy link
Collaborator

slorber commented Jun 10, 2020

Docs plugin: createDocsBaseMetadata(version) allows to pass data to the DocPage component, which is the layout component for a whole doc version.

I think we should add data here with a permalink to the latest docs homepage (not necessarily /docs, see routeBasePath)

But this is likely to conflict with my existing PR. You can make a POC, but can you wait before merging this?

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Honestly, this current solution does not bear any benefit. As I understand it, we need to clearly inform the docs user that they is looking at outdated information, and also encourage they to use the latest version.

The link in "breadcrumbs" is almost invisible (and especially in dark mode looks ugly, IMO).

I think we should add something like that alert:

screenshot_26

This way we inform the user clearly enough:

  • that they is viewing docs for an outdated version
  • we "urge" they to switch to the latest version of docs, thus prompting they to upgrade their website to the newest version of Docusaurus
Source code of snippet
<div class="alert alert--danger margin-bottom--md" role="alert">
  This is archived documentation for Docusaurus <strong>v2.0.0-alpha.50</strong>, which is no longer actively maintained.
  <br><br>For up-to-date documentation, see the <a href="#">latest version</a>.
</div>

@teikjun
Copy link
Contributor Author

teikjun commented Jun 10, 2020

Thanks a lot for the reviews! We'll make a POC following @lex111's solution, and wait for #2905 before merging.

@teikjun
Copy link
Contributor Author

teikjun commented Jun 11, 2020

Summary of Changes

  1. Changed the small badge to a banner.
  2. Replaced isOld with latestPermalink. latestPermalink will always link to the latest docs homepage.

Considerations
Regarding point 2, we considered linking older docs to the same doc in the latest version. But we couldn't find a good way to handle pages that have been renamed/deleted.
From our understanding, this requires looking up the list of currently available docs (so that we can link to latest homepage if the same doc does not exist). This would be less efficient compared to directly linking to homepage.

@teikjun teikjun requested review from slorber and lex111 June 11, 2020 10:21
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

That almost looks good to me, but the /docs path does not always exist on all sites.

If there's no "docs home", maybe we should link to the first doc of the sidebar instead?

Subjective: I'd personally prefer a warning/orange than an error/red for the banner color.

const latestPermalink =
version && version !== versioning.latestVersion
? normalizeUrl([baseUrl, routeBasePath])
: undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is almost what we want :)

on D2 website, we have a docs homePageId, so the root is /docs

        docs: {
          homePageId: 'introduction',
          path: 'docs',

But there are sites that do not have any docs home, it's the case for Watchman:

For sites like Watchman without a doc at the root, we'd rather make sure that we don't link to a path that does not exist

@teikjun
Copy link
Contributor Author

teikjun commented Jun 11, 2020

Summary of changes:

  1. Link on the banner goes to first doc of sidebar instead of doc homepage
  2. Changed banner color to warning orange

@teikjun teikjun requested a review from slorber June 11, 2020 16:53
@teikjun teikjun changed the title feat(v2): add a badge that links to latest version of documentation feat(v2): add a banner that links to latest version of documentation Jun 11, 2020
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

The algo should be more:

  • if there's a home doc id, the link should target that home doc id
  • else, link to the first document of the sidebar

@@ -461,6 +472,16 @@ Available document ids=
Object.values(content.docsMetadata),
'version',
);
const rootUrl = getHrefFromSideBar(
content.docsSidebars[`version-${versioning.latestVersion}/docs`],
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately the root url is not necessarily the first doc of the sidebar. If the user used a homeDocumentId, we must use it in priority, even if it's not the first doc in the sidebar (even if users usually use the first doc of the sidebar as home documentId).

@slorber
Copy link
Collaborator

slorber commented Jun 12, 2020

Hey,

I'd be fine if you fix the homeDocId problem to merge this PR.


Note, in the future it's probably needed we'll actually need to link to the same doc of the latest version, with a fallback to homeDocId, then firstDoc (of latest version)

This kind of behavior keeps being asked (yesterday here: #2923)

I think we'd need to provide some algo like

function getAllLinks(sidebarItems: DocsSidebarItem[]): SidebarItemLink[] {
 return flatten(sidebarItems.map(item => {
   if (sidebarItem.type === 'category') {
     return getAllLinks(sidebarItem.items);
   } else {
     return [sidebarItem.href];
   }
 }));
}

function getLatestDocsPermalink(links: SidebarItemLink[],preferedPaths: string[]): string | undefined {
 const existingPreferedPath = preferedPaths.find(preferedPath => {
   return links.some(link => link.href === preferedPath);
 })
 if ( existingPreferedPath ) {
   return existingPreferedPath;
 }
 else {
      return links[0]?.href;
 }
}

const docsLatestPermalink = getLatestDocsPermalink(latestSidebarData, [
  currentDocPath,
  homeDocIdPath
]);

This is really pseudo code, maybe you'll find it useful

@slorber
Copy link
Collaborator

slorber commented Jun 12, 2020

Didn't try, but this could work fine:

      const latestHomeDoc = options.homePageId ? content.docsMetadata[options.homePageId] : undefined;
      let latestDocPermalink: string
      if ( latestHomeDoc ) {
        latestDocPermalink = latestHomeDoc.permalink;
      }
      else {
        latestDocPermalink = getHrefFromSideBar(
          content.docsSidebars[`version-${versioning.latestVersion}/docs`],
        );
      }

try to test with homePageId: 'notTheFirstDoc', and see if it works :)

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

That looks good to me

Just, what about renaming rootUrl/latestPermalink to something more explicit? like latestVersionMainDocPermalink

Also not fan of getHrefFromSideBar what about getFirstDocLinkOfSidebar?

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Actually, there's a case where this fails, if the homeDocId selected does not exist in latest docs version.

As the "home" concept is currently not versioned, it is possible that the user selects a home that exists in next (ie /docs/next path will be created), but does not exist in current docs version (ie /docs will not be created)

Test with:

          homePageId: 'doesNotExist',

@anshulrgoyal
Copy link
Contributor

Actually, there's a case where this fails, if the homeDocId selected does not exist in latest docs version.

As the "home" concept is currently not versioned, it is possible that the user selects a home that exists in next (ie /docs/next path will be created), but does not exist in current docs version (ie /docs will not be created)

Test with:

          homePageId: 'doesNotExist',

I have a fix, I will push changes

@teikjun teikjun requested a review from slorber June 12, 2020 17:12
@slorber
Copy link
Collaborator

slorber commented Jun 15, 2020

LGTM, merging as #2905 is not reviewed yet

@slorber slorber merged commit 0c92f5a into facebook:master Jun 15, 2020
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Jun 17, 2020
@slorber
Copy link
Collaborator

slorber commented Jun 18, 2020

Released in https://github.com/facebook/docusaurus/releases/tag/v2.0.0-alpha.58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"These docs are out of date!" banner support for older docs pages
6 participants