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

[Redesign] Fix registration's link to the README #8699

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

loic-sharma
Copy link
Contributor

@loic-sharma loic-sharma commented Jul 22, 2021

Background

For packages with an embedded readme, registration's readmeUrl links to the display package page using the #show-readme-container URL fragment. For example: https://api.nuget.org/v3/registration5-gz-semver2/knapcode.torsharp/index.json:

Example registration JSON for package with embedded readme...
{
  ...

  "items": [
    {
      ...

      "items": [
        {
          ...

          "@id": "https://api.nuget.org/v3/registration5-gz-semver2/knapcode.torsharp/2.7.0.json",
          "catalogEntry": {
            ...
            "licenseUrl": "https://www.nuget.org/packages/Knapcode.TorSharp/2.7.0/license",
            "readmeUrl": "https://www.nuget.org/packages/Knapcode.TorSharp/2.7.0#show-readme-container",
            ...
            "version": "2.7.0"
          },
          ...
        },

        ...
      ],

      ...
    }
  ]

  ...
}

Fix

In the current design, this is implemented using Bootstrap 3's collapse component. The redesign broke this feature as it uses Bootstrap 3's tab component instead, which does not support opening using fragments. This change re-adds support for the #show-readme-container fragment link using Javascript.

I also fixed some ARIA attributes to improve accessibility.

Addresses #8698

@loic-sharma loic-sharma requested a review from a team as a code owner July 22, 2021 18:07
@loic-sharma loic-sharma changed the title Fix [Redesign] Fix registration's link to the README Jul 22, 2021
@joelverhagen
Copy link
Member

joelverhagen commented Jul 22, 2021

I think the expander section works generically with:

// If the URI fragment (hash) matches the expander ID, automatically expand the section.
if (document.location.hash === showId) {
hidden.collapse('show');
}

Would it make sense to do something similar for tabs?

@loic-sharma
Copy link
Contributor Author

@joelverhagen Nice catch, I didn't realize we had implemented that URL functionality.

Would it make sense to do something similar for tabs?

This would be useful, but this particular fix wouldn't use that general/shared logic. The URL fragment to open the README, show-readme-container, is specific to the naming scheme we use for collapsible sections (and changing this URL fragment would require rebuilding registration). Tabs use a different naming scheme: readme-tab, dependencies-tab, etc... If we add generic "open tab using URL fragment" logic, ideally it would work using this new naming scheme with a URL fragment like #readme-tab. So if we add generic logic, we would still need this "backwards compatibility" hack to support the existing registration API.

I'd suggest we punt the generic logic until we have a scenarios that would use this logic. Let me know if you feel otherwise @joelverhagen .

@@ -494,14 +494,25 @@
<ul class="nav nav-tabs" role="tablist">
@if (!Model.Deleted)
{
<li role="presentation" class="active"><a href="#readme-tab" aria-controls="ReadMe" role="tab" data-toggle="tab" id="readme-body-tab" class="body-tab">README</a></li>
<li role="presentation"><a href="#dependencies-tab" aria-controls="Dependencies" role="tab" data-toggle="tab" id="dependencies-body-tab" class="body-tab">Dependencies</a></li>
<li role="presentation" class="active" id="show-readme-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very sure. Do we need the id here?

Copy link
Contributor Author

@loic-sharma loic-sharma Jul 23, 2021

Choose a reason for hiding this comment

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

Yup, as the browser will move down the page to the element with the show-readme-container ID when I have the URL fragment #show-readme-container. (The .focus() in javascript does not do that, it simply highlights the element)

@loic-sharma loic-sharma merged commit ef13809 into dev Jul 23, 2021
@loic-sharma loic-sharma deleted the loshar-fix-readme-fragment branch July 23, 2021 21:22
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.

5 participants