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 sidebar sticky #140

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

milanmlft
Copy link

@milanmlft milanmlft commented May 29, 2024

Keeps the sidebar navigation menu visible when scrolling past it.
I mainly implemented this as a request for https://github.com/learntodiscover/varnish but thought I'd contribute it here as well. Happy to address any feedback!

Fixes #16

@ErinBecker
Copy link
Contributor

Thank you for this contribution @milanmlft! I'll take a look at it with @froggleston at one of our upcoming co-working meetings.

@milanmlft
Copy link
Author

Hi @ErinBecker! Hope you're doing well. I know you guys are super busy, but did you have a chance to look at this yet? Thanks!

@ErinBecker
Copy link
Contributor

Hi @milanmlft - Happy New Year! 🥳 I'm back at my desk with renewed percent effort to dedicate to Workbench and am finally reviewing your contribution. Thank you so much for your patience.

I've locally tested this PR and can confirm that the sidebar is indeed sticky! I personally agree that this is a great improvement to the lesson formatting and makes navigation much easier. After reading through some of the conversation on #16 and further issues referenced there, I want to do my due diligence and make sure this solution is compatible with some of the concerns raised there:

  1. @zkamvar raised a concern with the heaviness of adding additional JavaScript libraries. I have zero experience with JS and don't know how to test how much weight is added with this implementation. Any thoughts you have would be appreciated.

  2. In October, @froggleston implemented sticky sidebar for glossario using https://github.com/abouolia/sticky-sidebar. I'd like to hear @froggleston's thoughts on whether we should use the same implementation for Workbench as Glossario for consistency.

  3. On the Glossario PR referenced above, @Imran-imtiaz48 provided some accessibility feedback. I'm not clear whether this feedback is related to our implementation of sticky-sidebar, or if it is feedback on how sticky-sidebar itself works. If the later, we should perhaps consider using @milanmlft's implementation rather than sticky-sidebar.

Apologies for the inconclusive response after such a long delay. I'm back in the saddle and ready to start making progress towards implementing this improvement and will be following the conversation related to my questions above closely. @froggleston - please feel free to chime in.

@zkamvar
Copy link
Contributor

zkamvar commented Jan 10, 2025

@zkamvar raised a concern with the heaviness of adding additional JavaScript libraries.

FWIW, I don't think there are any new JS libraries in here, so that concern is taken care of.

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.

Sidebar behavior
3 participants