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

add sidebar toggle #862

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

FidalMathew
Copy link
Contributor

@FidalMathew FidalMathew commented Dec 10, 2024

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:

image image

For larger screens:

image

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:

    • The new toggle functionality allows users to show and hide the side navigation bar, with a close button to close it manually.
    • The overlay provides a better user experience by dimming the content when the sidebar is open in mobile view.
    • No breaking changes were introduced, so this update can be safely merged into existing applications.

Steps to test

  1. Go to home page
  2. Reduce window screen size
  3. Try to toggle using menu bar and close icon

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

@FidalMathew
Copy link
Contributor Author

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

@MisRob MisRob self-requested a review December 12, 2024 04:08
@MisRob MisRob added the TODO: needs review Waiting for review label Dec 12, 2024
@MisRob MisRob self-assigned this Dec 13, 2024
@MisRob
Copy link
Member

MisRob commented Dec 16, 2024

Hi @FidalMathew, thank you! Couple of high-level thoughts:

  • Yes, I think it makes sense to hide the sidebar by default only on smaller devices. We use the sidebar a lot so I like being it visible on larger devices, with the optional way to hide it. So overall all good!
  • Could we show the close icon above the logo in the very upper left corner of the sidebar?
  • On larger screens:
    • When the sidebar is visible, could you hide the hamburger icon next to the page title
    • Could you make the close button work in the sidebar?

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.

@FidalMathew
Copy link
Contributor Author

@MisRob Could you check it now. I have made the required changes

Copy link
Member

@AlexVelezLl AlexVelezLl left a 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:

image

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

docs/common/DocsPageTemplate/index.vue Outdated Show resolved Hide resolved
@AlexVelezLl
Copy link
Member

Also! Two checks are failing in your PR. Please run yarn lint-fix to fix the linting issues. And please, fill out the Changelog section of the PR description 👐. Thanks!

@FidalMathew
Copy link
Contributor Author

@AlexVelezLl I have made the required changes. Let me know if any further changes are required.

Copy link
Member

@AlexVelezLl AlexVelezLl left a 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?

image

Apart from that everything else looks good!

docs/common/DocsPageTemplate/index.vue Outdated Show resolved Hide resolved
@FidalMathew
Copy link
Contributor Author

@AlexVelezLl I have made the required changes. Could you check it out.

@AlexVelezLl
Copy link
Member

Thank you @FidalMathew! This looks good to me! Will left the final approval and merge to @MisRob 👐.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants