-
Notifications
You must be signed in to change notification settings - Fork 87
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 sidebar toggle #862
base: develop
Are you sure you want to change the base?
add sidebar toggle #862
Conversation
I'm planning to fix the icon visibility as well as adding overlay in smaller screen on the main content when the sidebar is visible. Any other suggestions ? @AlexVelezLl @MisRob |
Hi @FidalMathew, thank you! Couple of high-level thoughts:
I'd imagine the experience to be the very same on all screen sizes, the only difference being that on smaller ones, the sidebar would be hidden by default, and on larger ones it'd be visible by default. |
@MisRob Could you check it now. I have made the required changes |
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.
Hey @FidalMathew. Code changes looks good! Just gonna ask one little thing: Lets replicate the backdrop behaviour that we have in Kolibri when the menu is open:
I dont think we have a backdrop in KDS yet, so you can see Kolibri's Backdrop implementation for reference.
And also, lets gonna add a dropshadow %dropshadow-6dp
to the sidenav when we are in mobile view. If you dont know how to use the %dropshadow-Xp
please refer to [this](https://design-system.learningequality.org/styling/#definitions
Also! Two checks are failing in your PR. Please run |
@AlexVelezLl I have made the required changes. Let me know if any further changes are required. |
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.
Thanks @FidalMathew!! This looks good. Just one little thing: Could you please fix the alignmennt between the hamburguer and the page title, so they are centered?
Apart from that everything else looks good!
@AlexVelezLl I have made the required changes. Could you check it out. |
Thank you @FidalMathew! This looks good to me! Will left the final approval and merge to @MisRob 👐. |
Description
This PR fixes sidebar view problem by using a menu icon to open it in smaller devices and close button to close the sidebar as well. On larger screens, both main content as well as sidebar is visible. .
Issue addressed
This PR addresses issue #107
Addresses #PR# HERE
Before/after screenshots
For Smaller Screens:
For larger screens:
Changelog
Description: Improves docs mobile responsiveness by adding a menu hamburguer to open docs sidenav on mobile devices.
Products impact: bugfix
Addresses: N/A
Components: KDS Docs: SideNav, Header
Breaking: no
Impacts a11y: no
Guidance:
Steps to test
(optional) Implementation notes
At a high level, how did you implement this?
Does this introduce any tech-debt items?
Testing checklist
Reviewer guidance
Comments