Add toggle mechanism for docs.rs source sidebar #1464

GuillaumeGomez · 2021-08-12T15:15:16Z

Showing how it works in video:

Peek.2021-08-17.14-09.mp4

jyn514 · 2021-08-16T01:51:53Z

Hmm, why do you keep the icons in the sidebar when it's collapsed? They don't seem very helpful since many of them are the same, you can't tell different icons apart. Rustdoc just shows an arrow to expand the sidebar, it doesn't still let you click things:

jyn514 · 2021-08-16T01:55:20Z

static/source.js

@@ -0,0 +1,34 @@
+(function() {
+    function showSourceFiles(button, sideMenu, sourceCode) {
+        button.getElementsByClassName("pure-menu-link")[0].childNodes[0].textContent = "< ";


Hmm, could you maybe add a title-text of "collapse" (and "expand" for the other)? This seems not great for accessibility.

Actually, @ahicks92 do you know how this is normally done for screen-readers? I assume this will read out as "left arrow" which doesn't seem ideal.

I can't figure out how to get a test link out of the pr; is there a way to do that?

@ahicks92: I can't host this PR so not possible unfortunately. :-/

@jyn514: Adding a title attribute would fix this issue I guess.

I think you want aria-label. You'd also want aria-expanded I think. Unfortunately this is tentative: let's say that I'm 70% confident. Fortunately, I think most screen reader users would be able to figure this out even without changes, though improving this is definitely a good idea.

Not 100% on whether or not title is relevant, but probably not the end of the world to add one. I think that title ends up being togglable and counted as a tooltip or something like that, not entirely replacing the text of the button in all contexts.

As a followup to that note that aria-expanded if done right is going to make it obvious that the button expands or collapses something, and the common mistake is working the action into the label (e.g. "expand table of contents" as the label becomes "expand table of contents button collapsed" for example, which is irrelevant info).

Ah, it's IPv6 only. I can maybe throw cloudflare in front of it to get an IPv4 reverse proxy 🤔.

Using cloudflare as a reverse proxy is a little more annoying than I expected, but hopefully this works now: https://ipvold-docs-rs-dev.nemo157.com/crate/tokio-util/0.6.7/source/src/codec/decoder.rs

@ahicks92: Let's go for aria-label, title and aria-expanded. :)

That's not a button tag. Missed that in my review somehow. The labeling stuff only works on items of the right role. The easiest (read: bug-free) way here is to use a button tag outside the list instead, but if we're willing to possibly accept bugs on some platforms then adding role="button" might or might not work. Unfortunately screen readers get buggy around edge cases, and "li tagged as button inside a list" seems like one. I hacked it in via the inspector with Firefox+NVDA and it seems to work for me. If there's a reason we need to keep it this way rather than using a proper button we can try it and I'll do some more testing on the combinations I have access to.

note also that non-button buttons etc. like this need tabindex=0 for keyboard accessibility; you can't tab to them by default either.

I plan to use a button, for now it was mostly validating the UI (but I think it's ok now so I'll update it).

jyn514 · 2021-08-16T01:55:53Z

templates/style/base.scss

@@ -438,7 +438,7 @@ div.package-page-container {
            margin-top: 0;
        }

-        a.pure-menu-link {
+        .pure-menu-link {


Hmm, why did this change?

Because I wanted to use the style of this class in the "collapse" button.

The classes don't entirely work on a non-link, if you click and drag it selects the text inside instead of dragging a button around like the other links.

GuillaumeGomez · 2021-08-16T08:09:42Z

Hmm, why do you keep the icons in the sidebar when it's collapsed? They don't seem very helpful since many of them are the same, you can't tell different icons apart. Rustdoc just shows an arrow to expand the sidebar, it doesn't still let you click things:

Because I don't know otherwise where to keep the button to "expand" the sidebar.

Edit: I got an idea: simply hide the links and only keep the button.

templates/style/base.scss

GuillaumeGomez · 2021-08-17T12:11:18Z

Updated the UI a bit and added the aria attributes. I also updated the video to show what the new UI looks like.

Nemo157 · 2021-08-17T13:27:18Z

Updated my server, looks nicer to me 😁

https://fontawesome.com/v5.15/icons/chevron-left?style=solid and -right might look good next to the filetype icons?

GuillaumeGomez · 2021-08-17T13:47:29Z

Why not! I think @jyn514 didn't like just having "<"/">" but maybe I misunderstood. If they're happy with it, I will use the fontawesome icon instead.

GuillaumeGomez · 2021-08-18T09:51:22Z

I replaced with font-awesome icons and now use a button.

ahicks92 · 2021-08-18T14:05:06Z

If @Nemo157 updates their server I'll check it again. The code looks right, though do we know if it's valid HTML to put buttons inside list elements? I didn't think so, and "isn't valid HTML" is often a good proxy for breaks with some screen reader somewhere.

GuillaumeGomez · 2021-08-18T14:34:48Z

I tested this HTML on the w3c validator:

<!DOCTYPE html>
<html lang="en">
<head>
<title>hello</title>
</head>
<body>
<ul>
<li><button>yolo</button></li>
</ul>
</body>
</html>

No warning or error, so I guess it's fine. 😄

Nemo157 · 2021-08-18T14:38:05Z

@ahicks92 it's updated

ahicks92 · 2021-08-18T22:21:49Z

Yeah, that works. Maybe "hide" in the label/title, because "collapse source sidebar expanded" is a bit awkward, but that's very much a NIT.

GuillaumeGomez · 2021-08-19T08:26:16Z

@ahicks92 No it makes sense because I used "Show" in for expanding it. So I replaced "collapse" with "hide".

jyn514 · 2021-08-21T20:02:24Z

This looks good to me, but I realized after using it that I have another complaint: the "Hide files" button doesn't scroll with the content, so you can end up having to scroll back up to the top to collapse the sidebar:

Rustdoc does this by making the sidebar scrolling independent from the content scrolling, maybe we could copy that? Or would you prefer to leave that for a follow-up?

GuillaumeGomez · 2021-08-21T20:52:55Z

It's easy to do but I'd prefer to do it in a follow-up for a simple reason: it's a behaviour change. So I'd prefer for people to confirm whether they want it or not on this specific part. I'll send a PR for it once this one is merged.

GuillaumeGomez requested a review from jyn514 August 12, 2021 15:15

jyn514 reviewed Aug 16, 2021

View reviewed changes

Nemo157 reviewed Aug 16, 2021

View reviewed changes

templates/style/base.scss Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the toggle-source-sidebar branch from f02fbe4 to 9148411 Compare August 17, 2021 12:10

GuillaumeGomez force-pushed the toggle-source-sidebar branch from 9148411 to 7633294 Compare August 18, 2021 09:50

Add toggle mechanism for docs.rs source sidebar

bab13f5

GuillaumeGomez force-pushed the toggle-source-sidebar branch from 7633294 to bab13f5 Compare August 19, 2021 08:25

jyn514 merged commit 6338388 into rust-lang:master Aug 21, 2021

jyn514 mentioned this pull request Aug 21, 2021

"Hide files" toggle should be fixed so it isn't hidden after scrolling #1474

Closed

GuillaumeGomez deleted the toggle-source-sidebar branch August 21, 2021 22:16

GuillaumeGomez mentioned this pull request Sep 28, 2021

Modified document body to adjust left margin according to toggling on off of side bar in docs. rust-lang/rust#88301

Closed

Add toggle mechanism for docs.rs source sidebar #1464

Add toggle mechanism for docs.rs source sidebar #1464

Uh oh!

Conversation

GuillaumeGomez commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Aug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Aug 17, 2021

Uh oh!

Nemo157 commented Aug 17, 2021

Uh oh!

GuillaumeGomez commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Aug 18, 2021

Uh oh!

ahicks92 commented Aug 18, 2021

Uh oh!

GuillaumeGomez commented Aug 18, 2021

Uh oh!

Nemo157 commented Aug 18, 2021

Uh oh!

ahicks92 commented Aug 18, 2021

Uh oh!

GuillaumeGomez commented Aug 19, 2021

Uh oh!

jyn514 commented Aug 21, 2021

Uh oh!

GuillaumeGomez commented Aug 21, 2021

Uh oh!

Uh oh!

GuillaumeGomez commented Aug 12, 2021 •

edited

Loading

GuillaumeGomez commented Aug 16, 2021 •

edited

Loading

GuillaumeGomez commented Aug 17, 2021 •

edited

Loading