-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
Feat: Add sidebar state persistence #1121
Feat: Add sidebar state persistence #1121
Conversation
🦋 Changeset detectedLatest commit: 7a96a92 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Hello! Thank you for opening your first PR to Starlight! ✨
Here’s what will happen next:
-
Our GitHub bots will run to check your changes.
If they spot any issues you will see some error messages on this PR.
Don’t hesitate to ask any questions if you’re not sure what these mean! -
In a few minutes, you’ll be able to see a preview of your changes on Vercel 🤩
-
One or more of our maintainers will take a look and may ask you to make changes.
We try to be responsive, but don’t worry if this takes a few days.
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 @kitschpatrol! Sorry for the extremely long wait for a response here. Are you still interested in working on this PR or would you like me to take over? Looking into this functionality now and would have some feedback/suggestions if you’re interested!
Hey! No worries on the delay. I'm happy to work on it and of course would want any and all feedback and suggestions — but unfortunately time-wise I would have to delay picking it back up for a few weeks (Feb 26th), so no issues at all if you would rather take it over and get it released more quickly. 👍 |
Thanks — good to know! In that case, I’ll try to at least leave a round of feedback out of politeness, and if I have the chance before you do to address that, I’ll be happy to jump in 🙌 |
Hey @delucis, I'm back online. 👋 Happy to dig back into this PR if you have bandwidth for feedback. |
@delucis, @kitschpatrol: I'd love to see this get merged soon if it's ready. I added the fix to one of my sites and had to override several files 😅:
Let me know if I can help. Thanks. |
Hi @bdenham, yes I'm definitely standing by to respond to any feedback from the maintainers to keep this moving! (I think they're having a busy week. 😅) And hmm can you give any more info on all the overrides you said you needed? I just merged up this PR with |
No, your PR is fine. I'm referring to what you need to do right now, outside the Starlight codebase. That is, when you're just using the Starlight integration in your site. The dependencies stack up when you need to override the sidebar from the outside to add the change you're committing to the sidebar on the inside. :) |
Thanks again for this PR and your patience on this feature. In the end we realised there were quite a few additional considerations that meant we wanted to do this differently, so ended up implementing a separate approach in #2150. Nevertheless, appreciate you getting the ball rolling! The new feature will be available in v0.26 releasing later today 🚀 |
Description
Builds on an initial implementation by @duncanwerner.
I grabbed the most salient bits of the issue thread below:
Feature criteria
Possible improvements
getBoundingClientRect()
to determine if scrolling the sidebar is necessary. (Note that trustingscrollIntoView()
on its own without checking bounds first can result in unnecessary scrolling.)Still to do