Skip to content

Allow toggling sidebar navigator on/off, for large breakpoints #99

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 44 commits into from
Aug 3, 2022
Merged

Allow toggling sidebar navigator on/off, for large breakpoints #99

merged 44 commits into from
Aug 3, 2022

Conversation

dobromir-hristov
Copy link
Contributor

Bug/issue #, if applicable: 89564104

Summary

Allows toggling the navigator ON/OFF, for large resolutions. This is done through the Nav toggle icon or by dragging.
Users can drag the navigator all the way to the minimum allowed size. Once they pass 2x the minimum, the sidebar gets hidden.

The user can expand it back out, if they have not let go of the mouse button upon dragging OR by clicking on the nav toggle. (should work similar to Mail.app)

Hidden state is persisted in localStorage, so it wont re-open for users who have closed it once.

Dependencies

NA

Testing

Steps:

  1. Try to click on the toggle to open/close the navigator on Large screen sizes. Assert nothing bleeds out when hidden.
  2. Make sure tabbing across the page does not focus anything inside the navigator
  3. Refresh the page to assert its kept closed.
  4. Open the navigator and start dragging all the way until it stops. Now drag 2x the left space and assert its toggled off.
  5. Without releasing your drag, try to expand out again. It should snap to the smallest possible size again.
  6. If you drag until closed, it should persist in LS the closed state, and later allow you to toggle open again.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

@dobromir-hristov dobromir-hristov force-pushed the dhristov/r89564104-allow-toggling-sidebar-on-dekstop branch from fed271a to 6e91dcd Compare March 25, 2022 13:15
@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

@biamx3
Copy link

biamx3 commented Mar 25, 2022 •
edited by dobromir-hristov
Loading

This is a great start.

My biggest piece of feedback is we need to enable the sidebar side by side with the content in M viewports. I know this is a pain and you'll need to rework a lot of the logic, but when I designed that behavior, the sidebar button wasn't in scope.

S viewports should remain as they are. If you click on the sidebar icon it will bring up the modal with the filter on top.

My other feedback are small visual things:

  1. The sidebar icon should be figure-gray. It looks disabled right now.
  2. The sidebar icon should align with the items in the sidebar.
  3. Let's add a separator between the icon and the Documentation item of the breadcrumb.

Screen Shot 2022-03-25 at 6 32 33 PM

@dobromir-hristov
Copy link
Contributor Author

@biamx3 I updated the PR to your specs. Let me know how it looks.

marinaaisa
marinaaisa reviewed Mar 30, 2022
View reviewed changes
Copy link
Member

@marinaaisa marinaaisa left a comment

Choose a reason for hiding this comment

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

Nice job, congrats! 😄

It works pretty well, I left couple of comments tho:

  • I noticed that the Sidenav toggle's focus outline is wider than the hover style.

Screenshot 2022-03-30 at 22 52 55

  • The navigator head item's focus outline has been modified as well. It seems that in main is not working totally right either, but here is worse. Let me know if you prefer that I create another radar for this one.

Screenshot 2022-03-30 at 22 58 52

@dobromir-hristov
Copy link
Contributor Author

I noticed that the Sidenav toggle's focus outline is wider than the hover style.

Will try to fix that one.

The navigator head item's focus outline has been modified as well. It seems that in main is not working totally right either, but here is worse. Let me know if you prefer that I create another radar for this one.

Thats because I probably added overflow: hidden, so things dont shine through, when we toggle the navigator to the minimum size.

@dobromir-hristov
Copy link
Contributor Author

@marinaaisa those should be fixed now.

@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

marinaaisa
marinaaisa approved these changes Mar 31, 2022
View reviewed changes
Copy link
Member

@marinaaisa marinaaisa left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Dobri!

@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

Merge branch 'main' into dhristov/r89564104-allow-toggling-sidebar-on…
a6d1d6e 
…-dekstop

# Conflicts:
#	src/components/DocumentationTopic/DocumentationNav.vue
#	src/components/NavBase.vue
#	src/components/Navigator.vue
#	src/components/Navigator/NavigatorCard.vue
#	src/views/DocumentationTopic.vue
#	tests/unit/components/AdjustableSidebarWidth.spec.js
#	tests/unit/components/DocumentationTopic/DocumentationNav.spec.js
#	tests/unit/components/NavBase.spec.js
#	tests/unit/views/DocumentationTopic.spec.js
@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

Dobromir Hristov added 2 commits July 19, 2022 13:22
@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

@SamLanier SamLanier self-requested a review July 29, 2022 19:13
SamLanier
SamLanier approved these changes Jul 29, 2022
View reviewed changes
Copy link

@SamLanier SamLanier left a comment

Choose a reason for hiding this comment

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

Design approved.

Merge branch 'main' into dhristov/r89564104-allow-toggling-sidebar-on…
2a7f24a 
…-dekstop

# Conflicts:
#	src/components/Navigator.vue
#	src/components/Navigator/NavigatorCard.vue
#	src/views/DocumentationTopic.vue
@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

@dobromir-hristov
Copy link
Contributor Author

@marinaaisa can you give this one more look before I merge it?

marinaaisa
marinaaisa reviewed Aug 1, 2022
View reviewed changes
src/components/DocumentationTopic/DocumentationNav/LanguageToggle.vue
@@ -125,12 +125,12 @@ export default {
},
mounted() {
// on resize, re-calculate the width of the select.
const cb = debounce(async () => {
const cb = throttle(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain me this change? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Bea noticed the language toggle was not getting the new screen size, when we resize the page by dragging the adjustable sidebar. Debounce can skip the last function hit, while throttle will always fire the last function invocation.

marinaaisa
marinaaisa requested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@marinaaisa marinaaisa left a comment

Choose a reason for hiding this comment

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

Code looks good!
I just found couple of AX improvements:

  1. Focus state of the button to close the navigation should be a rectangle around the button. Now it looks like this Screenshot 2022-08-01 at 15 33 23

  2. Focus state gets lost when open and close navigator buttons are triggered. As a keyboard user I would expect to keep the focus state in the same element

focus-lost-when-button-pressed.mov

@dobromir-hristov
Copy link
Contributor Author

@marinaaisa first issue I can probably figure something out, though its another hack 😆 I hate overflow: hidden.

For the second one, what do you suggest we do? Should the focus move from one toggle to the other?

refactor: properly add .sidebar-transitioning class to the Adjustab…
ed74616 
…leSidebarWidth, when in transition.

Rename `.animating` to `.sidebar-transitioning`
@marinaaisa
Copy link
Member

For the second one, what do you suggest we do? Should the focus move from one toggle to the other?

Yes, I understand that there is two different toggles but the user only see one. We should keep the focus in the same one.

@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

marinaaisa
marinaaisa approved these changes Aug 2, 2022
View reviewed changes
Copy link
Member

@marinaaisa marinaaisa left a comment

Choose a reason for hiding this comment

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

Great AX improvements, @dobromir-hristov , thanks for your effort on it!

@dobromir-hristov dobromir-hristov merged commit 60fd7bf into swiftlang:main Aug 3, 2022
@dobromir-hristov dobromir-hristov deleted the dhristov/r89564104-allow-toggling-sidebar-on-dekstop branch August 3, 2022 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marinaaisa marinaaisa marinaaisa approved these changes

@SamLanier SamLanier SamLanier approved these changes

@mportiz08 mportiz08 Awaiting requested review from mportiz08

@hqhhuang hqhhuang Awaiting requested review from hqhhuang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dobromir-hristov @biamx3 @marinaaisa @SamLanier