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

control scrollbar styling #637

Merged
merged 10 commits into from May 11, 2022
Merged

control scrollbar styling #637

merged 10 commits into from May 11, 2022

Conversation

12rambau
Copy link
Collaborator

Fix #633

WIP

@choldgraf
Copy link
Collaborator

looks like a good start! lemme know when you'd like a review - I think we can also implement the "dark / light toggle" logic inside those mixins - even though this would now be in a different place from _colors.scss, I feel like it's more sensible to have it inside the mixins, what do you think?

@12rambau
Copy link
Collaborator Author

12rambau commented Apr 22, 2022

I can't really test it, I'm on a mac computer and my system is overriding all the navbars (that's also why I never noticed the problem). I'll test it over my FAO laptop on Monday to check the behavior on a Windows machine.

think we can also implement the "dark / light toggle" logic inside those mixin

I don't really see which logic you're talking about. there's not that much logic in the toggle apart from a double set of colors (which I think is just fine in the _colors.scss file). Unless you're talking about this (

html[data-mode="auto"] #theme-switch a[data-mode="auto"] {
)

@choldgraf
Copy link
Collaborator

Ah never mind about my comments on colors, I'm being silly and was thinking we'd have to define specific background and foreground colors for the scroll bars. Ignore my statement!

@choldgraf
Copy link
Collaborator

hmm - for some reason, on my screen the scrollbar is only showing up when I hover over the scrollbar area, not the whole sidebar...so something seems to be off with the "hover" event

@damianavila
Copy link
Collaborator

hmm - for some reason, on my screen the scrollbar is only showing up when I hover over the scrollbar area, not the whole sidebar...so something seems to be off with the "hover" event

I see the same behavior...

@12rambau
Copy link
Collaborator Author

12rambau commented May 3, 2022

Add a grey scrollbar to both the body and the primary sidebar. The scrolllbar is using the background color for the track (if not the body one uses a black background by default) and a grey/white coloring. I decided to remove any hover behavior because I don't think they are very helpful (open to discussion). happy to hear your feedbacks

@12rambau 12rambau marked this pull request as ready for review May 3, 2022 17:26
@choldgraf
Copy link
Collaborator

FYI this is looking pretty good to me now!

@choldgraf
Copy link
Collaborator

Could we apply the same styling to the in-page toc?

image

And it might be useful to do the same for other major page elements (e.g., code blocks and div elements with scroll), but that might be better for a subsequent PR if it'd be complicated

@12rambau
Copy link
Collaborator Author

12rambau commented May 4, 2022

And it might be useful to do the same for other major page elements (e.g., code blocks and div elements with scroll)

as it's a mixin function it's nothing really. I've never used div or code-block that are scrollable. Is there examples in the doc so that I can test it ? (I'm mainly looking for the css class/id)

@choldgraf
Copy link
Collaborator

Any of the code blocks with relatively wide content should demonstrate how horizontal scrollbars look. For example, narrow your screen and look here:

https://pydata-sphinx-theme.readthedocs.io/en/stable/demo/example_pandas.html

Though we could also provide an example in theme-specific-elements to show off the sidebars in an easier-to-find way

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really nice to me - I'm +1 on merging and iterating in future PRs if we find more things to improve here. WDYT?

@choldgraf choldgraf merged commit 5286126 into pydata:master May 11, 2022
@choldgraf
Copy link
Collaborator

OK let's merge this in and see how people like it!

@12rambau 12rambau deleted the scrollbar branch May 11, 2022 12:04
@damianavila
Copy link
Collaborator

I like it!!!

@jarrodmillman jarrodmillman added this to the 0.9 milestone May 25, 2022
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.

Improve scroll bars and support dark mode
4 participants