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

Add warning for stale version of documentation #2012

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

alisinabh
Copy link
Contributor

Continuation of #1918 (could not push to the same branch since I'm not a maintainer)


Based on feedback in #1918 here is the revised version of warning color and icon added when an outdated version is detected.

@halostatue I tried yellow but I could not make it work on Light mode. It would end-up look green-ish when lightness was reduced.

Screenshots

Screenshot 2025-01-08 at 2 30 25 PM Screenshot 2025-01-08 at 2 30 37 PM Screenshot 2025-01-08 at 2 30 45 PM

NOTE: Initially I wanted to put the icon after the version, but the <select> HTML element width is set to the largest possible item. This will create an unwanted padding that looks ugly in projects which have versions like 1.2.3-RC0.

@halostatue
Copy link

Screenshot 2025-01-08 at 14 49 22
Screenshot 2025-01-08 at 14 49 28
Screenshot 2025-01-08 at 14 49 38
Screenshot 2025-01-08 at 14 49 44
Screenshot 2025-01-08 at 14 49 51

assets/js/sidebar/sidebar-version-select.js Outdated Show resolved Hide resolved
Comment on lines 5 to 8
{{#if latestVersion}}
<i class="ri-alert-line sidebar-staleIcon" aria-hidden="true"></i>
{{/if}}
<select class="sidebar-projectVersionsDropdown{{#if latestVersion}} sidebar-staleVersion{{/if}}"{{#if latestVersion}} title="This version is not the latest version of the package."{{/if}}>

Choose a reason for hiding this comment

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

I think that this should be both more SR friendly and mobile friendly with the use of a popover. I am thinking:

Suggested change
{{#if latestVersion}}
<i class="ri-alert-line sidebar-staleIcon" aria-hidden="true"></i>
{{/if}}
<select class="sidebar-projectVersionsDropdown{{#if latestVersion}} sidebar-staleVersion{{/if}}"{{#if latestVersion}} title="This version is not the latest version of the package."{{/if}}>
{{#if latestVersion}}
<i class="ri-alert-line sidebar-staleIcon" aria-hidden=true popovertarget="staleVersion"></i>
<span class="sr-only">This is not the latest version of this package.</span>
<div id="staleVersion" popover>This is not the latest version of this package.</div>
{{/if}}
<select class="sidebar-projectVersionsDropdown{{# if latestVersion}} sidebar-staleVersion{{/if}}">

I’m not sure if this will work, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since popover works with clicks and not hover, this won't work as clicking on the icon will open the version selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SR text addressed in da05c1d

Copy link

github-actions bot commented Jan 8, 2025

@alisinabh alisinabh requested a review from halostatue January 8, 2025 20:59
@josevalim
Copy link
Member

josevalim commented Jan 9, 2025

Sorry, a couple additional thoughts (we are going in circles here and I apologize):

  1. The most likely thing the user will want to do is to jump to latest

  2. Both --orange and --orangeDark today in light mode today have low AA accessibility rating on Firefox

So my suggestion is:

  1. have --orangeDark be hsl(30, 90%, 40%) and use it on light mode
  2. have --orangeLight be hsl(30, 80%, 50%) and use it on dark mode
  3. Add a link to jump to latest

Here is a quick HTML I came up with:

<div style="display: inline;padding-left: 8px;color: hsl(30, 90%, 40%);">
  <i aria-hidden="true" class="ri-alert-line"></i> <span>Go to latest</span>
</div>

And here is how it looks like:

PNG image

WDYT?

@halostatue
Copy link

Sorry, a couple additional thoughts (we are going in circles here and I apologize):

  1. The most likely thing the user will want to do is to jump to latest
  2. Both --orange and --orangeDark today in light mode today have low AA accessibility rating on Firefox

So my suggestion is:

  1. have --orangeDark be hsl(30, 90%, 40%) and use it on light mode
  2. have --orangeLight be hsl(30, 80%, 50%) and use it on dark mode
  3. Add a link to jump to latest

You mean something like this?

<div class="sidebar-projectVersion" translate="no">
  <form autocomplete="off">
    <label>
      <span class="sidebar-projectVersionsDropdownCaret" aria-hidden="true"></span>
      <span class="sr-only">Project version</span>
      <select class="sidebar-projectVersionsDropdown">
          <option translate="no" value="https://hexdocs.pm/elixir/1.18.1">v1.18.1</option>
          <option translate="no" value="https://hexdocs.pm/elixir/1.18.0">v1.18.0</option>
          <option translate="no" value="https://hexdocs.pm/elixir/1.18.0-rc.0">v1.18.0-rc.0</option>
          <option translate="no" value="https://hexdocs.pm/elixir/1.17.3">v1.17.3</option>
      </select>
    </label>
  </form>
  <div style="display: inline;padding-left: 8px;color: hsl(30, 90%, 40%);">
    <i aria-hidden="true" class="ri-alert-line"></i> <span>Go to latest</span>
  </div>
</div>

@halostatue
Copy link

FWIW, I can't seem to get it to work well on Safari with anything except beneath the select or form (so the inline can go away).

Screenshot 2025-01-09 at 10 38 45

@josevalim
Copy link
Member

Unfortunately it needs to after the version dropdown, otherwise will break the logo positioning in several designs.

@josevalim
Copy link
Member

josevalim commented Jan 9, 2025

I understand the issue. The width of the select is dictated by its largest option, since we have options with -rc1, there is extra padding. We would need to use some JavaScript to adjust it. Here is what ChatGPT proposed:


    const adjustWidth = () => {
      // Create a temporary element to measure the width
      const temp = document.createElement('span');
      temp.style.visibility = 'hidden';
      temp.style.position = 'absolute';
      temp.style.whiteSpace = 'nowrap';
      temp.style.font = window.getComputedStyle(select).font;
      temp.textContent = select.options[select.selectedIndex].text;

      document.body.appendChild(temp);
      select.style.width = `${temp.offsetWidth + 20}px`; // Add some padding
      document.body.removeChild(temp);
    };

But I didn't try it. We could run this before we inject the select.

@josevalim
Copy link
Member

I tested it on iOS and it worked well, so we are good with going this route. However I have found two bugs:

  1. The select has font-weight of 400 but the text currently does not have
  2. Clicking on the caret does not open up the select, perhaps we should move the caret inside the text?

@alisinabh alisinabh force-pushed the add-version-warning branch from 1c9f7c5 to a1abbec Compare January 9, 2025 17:38
@alisinabh
Copy link
Contributor Author

@josevalim Sorry I was testing my solution and realized it does not work on Safari (needs two clicks since safari does not allow showPicker in some conditions). Tried what you posted and it worked better also the logic is more simple.

Can you please test again?

@alisinabh alisinabh force-pushed the add-version-warning branch from a1abbec to c77a62f Compare January 9, 2025 17:45
@alisinabh alisinabh requested a review from josevalim January 9, 2025 17:47
assets/css/sidebar.css Outdated Show resolved Hide resolved
@josevalim
Copy link
Member

@alisinabh I have dropped two last comments and this is good to go! Please rebase the PR and submit it so it doesn't contain changes the formatter directory. Thank you so much! ❤️

alisinabh and others added 2 commits January 9, 2025 13:21
Co-Authored-By: Philip Davies <philip.davies@thekeysupport.com>

Rename latestVersion to isStale and use boolean

Co-Authored-By: Austin Ziegler <austin@zieglers.ca>

Revert "Rename latestVersion to isStale and use boolean"

This reverts commit 8ef47f0.

Use "Go to latest" link

Use actual URL for Go to latest link

Adjust select width

Fix openVersionSelect function

Revert "Adjust select width"

This reverts commit 5835e2e.

Use width calculation select width

Update assets/css/sidebar.css

Co-authored-by: José Valim <jose.valim@gmail.com>

Update assets/css/sidebar.css

Co-authored-by: José Valim <jose.valim@gmail.com>
@alisinabh alisinabh force-pushed the add-version-warning branch from db42bd3 to 5dd3ff6 Compare January 9, 2025 18:25
@alisinabh
Copy link
Contributor Author

alisinabh commented Jan 9, 2025

@josevalim Thank you. Rebased and ready to go.

@halostatue Thanks for your help and input :)

Final screenshots

Screenshot 2025-01-09 at 1 28 31 PM Screenshot 2025-01-09 at 1 28 23 PM

@josevalim josevalim merged commit d83e5bc into elixir-lang:main Jan 10, 2025
4 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

select.style.width = `${temp.offsetWidth + 20}px`
document.body.removeChild(temp)
}

function handleVersionSelected (event) {
Copy link
Member

Choose a reason for hiding this comment

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

handleVersionSelected makes sure that when we are on, say, https://hexdocs.pm/elixir/1.18.0/Task.html and from version selector we choose v1.18.1, we'll end up at https://hexdocs.pm/elixir/1.18.1/Task.html.

I think it'd be nice for "Go to latest" to work similarly, preserve relative path.

I'm not very well versed in JS but this is a PoC (copy pasting the implementation with minor changes) for this functionality:

diff --git a/assets/js/sidebar/sidebar-version-select.js b/assets/js/sidebar/sidebar-version-select.js
index 48968c88..de80813f 100644
--- a/assets/js/sidebar/sidebar-version-select.js
+++ b/assets/js/sidebar/sidebar-version-select.js
@@ -34,6 +34,9 @@ if (!isEmbedded) {
     const select = qs(VERSIONS_DROPDOWN_SELECTOR)
     select.addEventListener('change', handleVersionSelected)
     adjustWidth(select)
+
+    const versionsGoToLatest = qs(".sidebar-staleVersion a")
+    versionsGoToLatest.addEventListener('click', handleGoToLatestClicked)
   }
 }

@@ -67,6 +70,22 @@ function handleVersionSelected (event) {
     })
 }

+function handleGoToLatestClicked (event) {
+  const url = this.href
+  const pathSuffix = window.location.pathname.split('/').pop() + window.location.hash
+  const otherVersionWithPath = `${url}/${pathSuffix}`
+  event.preventDefault();
+
+  checkUrlExists(otherVersionWithPath)
+    .then(exists => {
+      if (exists) {
+        window.location.href = otherVersionWithPath
+      } else {
+        window.location.href = url
+      }
+    })
+}

@alisinabh if this piques your interest perhaps you could send another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants