-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix #1177: Usability problem with sidebar on mobile devices #1182
base: master
Are you sure you want to change the base?
Conversation
sphinx_rtd_theme/layout.html
Outdated
|
||
{#- MOBILE NAV, TRIGGLES SIDE NAV ON TOGGLE #} | ||
<div class="wy-close-top"> | ||
<i data-toggle="wy-nav-top" class="fa fa-close"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make this more accessible? I wasn't really sure what hitting the X would do when I tried to use it -- perhaps even some text along with the X
like Close Sidebar X
? I'm guessing there's also an aria-label
we should be using here for screen readers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a close button is the example in these docs, so I think we should definitely include one: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-label_attribute#examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-label
is the best solution here, yeah. Verbose text would fill out too much space and would need to be translated, which introduces other horizontal spacing concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder about something a bit more explicit that it's closing the menu, but I agree that translation point is a good one. Do the aria labels also need to be translated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, aria elements should also be translated. The theme actually has pretty active translations, so localization is definitely a bit more effective here.
Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great upgrade, the 85% menu always bothered me a bit.
Do you think there is anything we can easily to do put the close button at the top of the navigation pane?
The top left position would position the close button right over where the hamburger menu was before opening the menu, though top right also feels intuitive.
I don't feel like there are good options that don't either 1) bump the content below the button down awkwardly or 2) potentially overlap elements if we make the close button float or fixed position. Neither are great, really. This PR could be a good interim location for the close button, but might not be as discoverable.
Ultimately, I could see us moving towards a menu that includes the top navigation bar as well. It would be nice to preserve the project title in both views probably. Something like:
This would most certainly be a larger change though, and probably not something to jump right into unless it's easy. The HTML structure for the top bar and the side navigation probably makes this a bit more work.
We can continue discussion separately if we want to do this in multiple steps.
@@ -88,7 +88,7 @@ | |||
|
|||
+media($tablet) | |||
.rst-versions | |||
width: 85% | |||
width: 100% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% width on the menu and content is way better 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realme edit
Also, going to wait to merge until 1.0 is tagged. |
Co-authored-by: Anthony <[email protected]>
I agree that the top of the navigation pane would be the best place for the close button.
There are several ways to solve these but all involve more design decisions.
👍 Completely agree! A better UX could definitely be accomplished here by redesigning the top pane and integrating the close button. |
Sounds like we have a plan then. It definitely does seem like a larger project to do this the way we want, we can address this in stages though.
These are both good points. I actually almost posted a version with the logo removed. It might be worth considering removing the logo in the mobile menu. It's already hidden in the menu and it feels to me to really interfere with usability of the menu on mobile. We have a horizontal logo too. For projects with square or vertical logos the menu gets even more cramped on mobile. |
I want to point out that I also made changes that work towards improving this as part of much larger refactors/rewrites. See: #1122 The PR listed above makes several changes to HTML structure, layout, and breakpoints. Making it a slightly bigger change but also a slightly different UX compared to this patch. Can you two look at the UX and tell me what you think? I can extract those changes from that PR if we like that UX more than this one. The one benefit of this patch is that it removes the hamburger icon, which is a plus because the icon is rather confusing and is being removed across global UI design patterns. |
Definitely and option. Or maybe have a second logo version, different from the desktop menu.
Removing the hamburger button is a big plus! I don't think that making the sidebar a fixed width would be the best option here. |
I commented on #1122 -- changing the HTML structure of the navigation is tricky because we have customer and users restyling our theme and expecting the existing current HTML structure. We are already discussing a big backwards incompatible change with bootstrap, and users will have to already port their styles to new HTML and classes once, I'd hate to make that twice for users. I think we'll be in a better place to make better UX on some of the underlying structure after we have dropped some older Sphinx versions, we only have HTML5 output to worry about, and we are done messing around with the main HTML structures with a Bootstrap port. Until then, I think we're mostly in maintenance mode to get a final release out supporting old Sphinx/etc. |
We'll be bumping the Sphinx requirements for 2.0, so perhaps that's a good milestone for this? |
Changes
(before it was set to 85% width so the original hamburger button that toggles the sidebar could still be accessed with the menu opened. now the close button was added inside the menu itself, so there's no need to access that button while the menu is opened)
.rst-versions
100% width accordingly.See bellow screenshots of before and after changes, both displaying the full window with the menu open
Before:
After:
Fixes #1177