-
-
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
fix: add right sidebar scrollbar (#1746) #1770
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: de7ce88 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 |
@BryceRussell @HiDeoo - Thanks for sharing the screenshots on other devices/browsers and for reviewing the PR in Talking and Docs, seems like a great way to collect feedback! Regarding @HiDeoo's thoughts:
Look forward to hearing more thoughts on this and happy to make any adjustments to the PR, thank you! |
I'll add that, especially given the feedback shared thus far, my recommendation would be to default to having scrollbars when content overflows (as opposed to a config option since user could override via CSS if they want to disable) and add "thin" styling to the scrollbar, defaulting to standard thin ( |
Adding a small reminder for myself as I forgot to do that today: I would like to see what is the end result with |
Just got a screenshot based on this webpage from someone using for accessibility purpose a scrollbar related extension:
If I am not forgetting anything, this pretty much leaves us with the current implementation used in this pull request. |
Hi @HiDeoo - Happy to take this in either direction as you see best, however my guess is that in this particular case, this is an issue in the extension not having caught up with the spec as Even Chrome which previously didn't support Would be curious if the extension used in the screenshot handles the If we did implement |
I just tried with I'm also not quite sure to understand how a fallback approach would work in this case as The extension in the screenshots is Rescroller but maybe I missed something or misunderstood the idea? |
Hi @HiDeoo - Thanks for the follow-up and testing some situations out. When using the If you take a look at the CSS of the POC I built, you'll see various rules configured based on the options in the Control Panel. If you toggle the "Scrollbar Type" field to "prefer webkit", the scrollbars will change to webkit style falling back to I took a look at the Rescroller extension and determined why it doesn't work when You can see this in action as follows:
You can run a similar test with the POC. In short, the extension has a bug and does not account for If we did apply @media (pointer: fine) {
:root {
--sl-scrollbar-size-legacy: 7px;
--sl-scrollbar-color-track: var(--ec-frm-edBg);
--sl-scrollbar-color-thumb: var(--ec-frm-edTabBarBrdBtmCol);
--sl-scrollbar-color-thumb-hover: var(--sl-color-gray-4);
}
.right-sidebar,
#starlight__sidebar {
scrollbar-width: thin;
}
/* Legacy browsers with `::-webkit-scrollbar-*` support */
.right-sidebar::-webkit-scrollbar,
#starlight__sidebar::-webkit-scrollbar {
height: var(--sl-scrollbar-size-legacy);
width: var(--sl-scrollbar-size-legacy);
}
.right-sidebar::-webkit-scrollbar-track,
#starlight__sidebar::-webkit-scrollbar-track {
background: var(--sl-scrollbar-color-track);
border-radius: 10px;
}
.right-sidebar::-webkit-scrollbar-thumb,
#starlight__sidebar::-webkit-scrollbar-thumb {
background: var(--sl-scrollbar-color-thumb);
border-radius: 10px;
}
.right-sidebar::-webkit-scrollbar-thumb:hover,
#starlight__sidebar::-webkit-scrollbar-thumb:hover {
background: var(--sl-scrollbar-color-thumb-hover);
}
} All the above said, I think the decision to apply |
Description
toc
gives no visual indication its scrollable #1746Adds a scrollbar to
right-sidebar
for accessibility and navigation. See details & discussion in right navtoc
gives no visual indication its scrollable #1746.Yes, scrollbar added.
Before:
After:
Additional Information
sl-container
had a width specified but then a max-width set whenmin-width: 72rem
when it only appears that the entire component would ever be visible atmin-width: 72rem
. I needed to ensurewidth: auto
now that the parent (.right-sidebar
) has a max-width, otherwise a horizontal scrollbar would display which wasn't necessary so to minimize risk in case I'm overlooking something, I just set width toauto
whenmin-width: 72rem
to be consistent with styles applied on.right-sidebar
. Based on what I'm seeing in this component, the both thesl-container
width andmax-width
could likely be removed completely but decided to play it safe - let me know if you think these can be removed.right-sidebar
is to provide spacing away from the window scrollbar so they don't end up immediately next to each other.