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

Make the sidebar resizable #214

Closed
wants to merge 4 commits into from
Closed

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Jan 25, 2025

Addresses #209

  • Sidebar is no longer collapsible but is instead resizable with a slider
  • Bumps the width to 250px (happy to bikeshed the width)
  • Uses flexbox for the sidebar content and sidebar button instead of floats
  • Adds some ARIA-specific attributes to the slider

Feedback welcome!

Preview: https://python-docs-theme-previews--214.org.readthedocs.build/en/214/

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

I wonder if we should have a min and/or max width, or let the user have a tiny or massive sidebar if they really want?

I will note that PR this does make it impossible to completely collapse the sidebar, which is possible by clicking the divider at https://docs.python.org/3/. Is that something we want to keep?

python_docs_theme/static/sidebar.js_t Outdated Show resolved Hide resolved
@tomasr8
Copy link
Member Author

tomasr8 commented Jan 26, 2025

I wonder if we should have a min and/or max width

I agree, it might be nicer to have some limits. I'll add some so we can try it out

I will note that PR this does make it impossible to completely collapse the sidebar, which is possible by clicking the divider at https://docs.python.org/3/. Is that something we want to keep?

I removed it because I felt it wouldn't really have much of a purpose anymore, though if people prefer both we could make it work :)

@tomasr8
Copy link
Member Author

tomasr8 commented Jan 26, 2025

I added a min/max width for the sidebar (200px minimum and a half of window.innerWidth maximum but we can refine the values)

@tomasr8 tomasr8 requested a review from hugovk January 26, 2025 22:25
@tomasr8 tomasr8 requested a review from hugovk January 27, 2025 20:53
@hugovk
Copy link
Member

hugovk commented Jan 28, 2025

Left: with JS
Right: without JS, still showing the "«" symbol, making it look collapsible, but it's not. Can we remove it?

image

@tomasr8
Copy link
Member Author

tomasr8 commented Jan 28, 2025

Right: without JS, still showing the "«" symbol, making it look collapsible, but it's not. Can we remove it?

Definitely, I need to get into the mindset that JS might not be available 😅 Currently I'm removing it through JS, but a CSS rule should work even better. I'll fix it later today

@ezio-melotti
Copy link
Member

ezio-melotti commented Jan 28, 2025

I'm not sure making the sidebar resizable is the right solution to the problem described in #209.

The problem I was solving when I made the sidebar collapsible many years ago was to improve the experience on small screen (mostly phones), where the sidebar was taking up over half of the screen. Making it collapsible is also useful on regular screens when the user doesn't need it and wants more space for the actual documentation.

This PR takes away the collapsibility and replaces it with resizeability which is IMO much less useful.

Going back to the original problems listed in the issue:

  1. Content doesn't always fit
    • I think a better solution for this would be to use CSS ellipsis possibly combined with a title="" or something similar to show the full text on hover (even though last time I checked there wasn't an easy way to achieve this with CSS only). I'd also argue that this is functionally a non-issue and at best an aesthetic one, since the visible part is most often enough to recognize what you are looking for (assuming you are not already using ctrl+f). Changing overflow and/or word-wrap rules, increasing sidebar width, decreasing text-indent, reducing font-size are also viable solutions.
  2. Sidebar is too narrow on wide screens
    • This could be solved either with a %-based width, or with media queries

I'd would be ok with a resizable sidebar if it didn't affect collapsibility, but offering both features might just end up complicating the UI for little gain IMO. Also note that dragging is more difficult and error-prone than just tapping to (un)collapse, especially on touchscreens.

@hugovk
Copy link
Member

hugovk commented Jan 28, 2025

The problem I was solving when I made the sidebar collapsible many years ago was to improve the experience on small screen (mostly phones), where the sidebar was taking up over half of the screen.

At least since the theme was made responsive (in #46) the sidebar is hidden on screens narrower than 1024 pixels, so this part shouldn't be a problem any more.

Going back to the original problems listed in the issue:

I'm more favourable to making the sidebar wider than trying to display more stuff in a small space. Screens are getting wider.

Truncating text has usability/accessibility issues (can't ctrl+f), as does showing text on hover (can't hover on touch devices, there's more accessibility concerns).

@ezio-melotti
Copy link
Member

Truncating text has usability/accessibility issues (can't ctrl+f), as does showing text on hover (can't hover on touch devices, there's more accessibility concerns).

Note however that if you are ctrl+fing something:

  1. even if it doesn't find it in the sidebar, it will find it directly in the page (which is even better).
  2. if the name is so long that it doesn't fit in the sidebar, chances are that you aren't going to type it all before getting a match (or failing to get one in the sidebar because you typed the part that doesn't fit).

I also checked the example linked in the original issue and:

  • the sidebar already seems to implement overflow-wrap: break-word, solving the truncation problem;
  • the depth of the sidebar seems to have been reduced, so that -- at least in that page -- there are no overflowing words anymore.

sidebarbutton.role = "slider"
sidebarbutton.title = _("Resize sidebar")
sidebarbutton.style.cursor = "col-resize" // Set the cursor only if JS is enabled
sidebarbutton.setAttribute("aria-label", _("Resize sidebar by dragging"))

Choose a reason for hiding this comment

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

Noting that aria-label should only be used like so to provide additional usage context to assistive technology users.
After testing this it seems the resizing function is only operable via dragging (mouse or similar) so it seems this aria-label is used as a "catch all" to provide usage instructions, thus defeating the purpose of ARIA, it might be best removing it altogether, and instead add a comment.

window.localStorage.setItem("sidebar", "collapsed")
}
sidebarbutton.innerHTML = ""
sidebarbutton.tabindex = "0" // make it focusable

Choose a reason for hiding this comment

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

Noting that I could not focus this while navigating with a keyboard nor through screen reader so there might be something tripping this.

@trallard
Copy link

I agree with @ezio-melotti here, I am not convinced this approach addresses the issues from the original issue. I think a much simpler alternative would be to make the sidebar wider (with a minimum width or similar) while keeping the collapsing function as this is a feature folks used when multitasking or in smaller screens.

The slider/splitter added in this PR adds a lot more complexity, and is only usable with a mouse so adds a new layer of accessibility concerns. There would be significant changes needed to make it keyboard and assistive tech operable and we might need keeping the collapsing feature anyway leading to a complex UI which might prove tricky to get right.

(Note that the collapse button as is needs some rework to make it operable with multiple inputs, but we can work on that separately but would take a lot less work than making an accessible slider + accessible collapse button).

@tomasr8
Copy link
Member Author

tomasr8 commented Jan 28, 2025

Noting that I could not focus this while navigating with a keyboard nor through screen reader so there might be something tripping this.

Strange, it's working fine for me locally. Maybe some issue with the preview?

Anyway, thanks for your input! I see your points and I agree that adding a slider might be too disruptive to some workflows. Personally, I'd also be happy with just increasing the (max-)width of the sidebar so that it's a bit more flexible.

[...] this is a feature folks used when multitasking or in smaller screens.

Out of curiosity, what kind of set up are we talking about? The sidebar is hidden for widths <1024 pixels and for screens larger than that there's enough space for the text even with the sidebar.

In any case, if nobody disagrees, I can close this PR and open one that simply makes the sidebar wider :)

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.

4 participants