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 a "thin" horiz scrollbar #1461

Closed

Conversation

nitzan-shaked
Copy link
Contributor

@nitzan-shaked nitzan-shaked commented Dec 31, 2022

First, a screenshot:

Screen Shot 2022-12-31 at 21 12 24

I don't like the horizontal scrollbar; even at "size" 1 it is still too fat. So I added an option for a "thin" version, refactoring ScrollBarRender along the way.

I didn't presume to know how you'd like the CSS to behave, so I went for the simplest I could think of: add a new "scrollbar-thin" boolean style. I am not married to the css design around it, I really only want the "thin" version.

There's no need to do anything special for a "thin" vertical scrollbar: imo it looks very nice at width 1.

@nitzan-shaked
Copy link
Contributor Author

I also changed the demo to show it off -- the DataTable has a thin scrollbar.

@willmcgugan
Copy link
Collaborator

A scrollbar size of 1 presents a too small click target when zoomed out. It also makes the horizontal and vertical bars a different side of you set them both to 1.

You can easily change it via CSS. See https://textual.textualize.io/styles/scrollbar_size/

@nitzan-shaked
Copy link
Contributor Author

nitzan-shaked commented Dec 31, 2022

Possibly so, but sometimes (e.g. when not zoomed out) I think it's the perfect size.
Also, often I don't actually click -- I want it as a visual aid, but I scroll with the mouse.

"But you can't scroll left/right with the mouse, and you changed the horizontal scrollbar, so that makes no sense", I hear you say. (I have good hearing). Well I also created a PR that makes MouseScrollUp and MouseScrollDown with modifiers, so I use ctrl-wheel to scroll left/right.

Finally -- what you wrote about easily changing with CSS -- makes me think maybe my descr on this PR isn't so good. Maybe I should attach a screenshot?

(EDIT: added a screenshot)

@willmcgugan
Copy link
Collaborator

Ok, I see what you've done now. I can see why you might like this, but there are some downsides.

Because you're using half blocks, you have less sub-cell resolution. With the current scrollbar characters, you have 8 characters which can give the scrolling a pixel perfect look. Not a deal breaker, but it is a downside.

The CSS changes don't fit with conventions. You won't see a true or false value in browser CSS. You would have something like scrollbar-variant: thin which leaves the possibility of having multiple different styles, not just regular and thin.

The thin scrollbars scrollbar-size don't work together. I'm guessing from the code that thin overrides size, which would no-doubt cause confusion in the future. CSS rules tend to be independent and not contradict each other like this.

So this is not a change I would want to accept right now. However, as a compromise I would accept some mechanism to substitute a different scrollbar renderable in code.

I might be open to the possibility of adding these thin scrollbars down the line, if we can solve the contradictory rules issues. Perhaps scrollbar-size: 0.5 1

@nitzan-shaked
Copy link
Contributor Author

I agree with all your comments, and as a matter of fact suspected you'd say them. Which is why I wrote that I'm completely not married to the css "design", and only wish for the thin scrollbar to exist. I obviously agree there's a downside to the the half-blocks in terms of resolution, but also agree it should be a user choice so not a deal breaker.

I'm very happy you propose a "plugin renderer" as a solution, because I figured that unless you accept sort-of as is then that is the right path to go.

With all that in mind: I'm happy to conjure something up but just want to say ahead of time:

If we go with the plugin approach then the way I see it that is something that the application writer would "configure". Presumably register a ScrollBarRender-derived class against a "scrollbar style". So the styles would be strings, and the CSS will be such. So while I like "scrollbar-size-horizontal: 0.5" it doesn't make much sense if we go with "scrollbar-variant: thin".

If you have a better idea I'm happy to accommodate. If you think the above sounds reasonable then I'll go ahead and do that.

Finally, just nothing that a such a renderer class would also be responsible for measuring the resulting scrollbar, for the sake of all those methods that need that (basically: all the measuring methods I changed to take "thin" into account).

Agreed?

I don't like the horizontal scrollbar; even at "size" 1 it is still too
fat. So I added an option for a "thin" version, refactoring
ScrollBarRender along the way.

I didn't presume to know how you'd like the CSS to behave, so I went for
the simplest I could think of: add a new "scrollbar-thin" boolean style.
I am not married to the css design around it, I really only want the
"thin" version.

There's no need to do anything special for a "thin" vertical scrollbar:
imo it looks very nice at width 1.
@willmcgugan
Copy link
Collaborator

You might want to open a discussion with a proposal before doing any more work.

@nitzan-shaked nitzan-shaked mentioned this pull request Jan 5, 2023
3 tasks
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.

None yet

2 participants