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

Scroll and scrollbar padding and rounding #4559

Closed
wants to merge 3 commits into from

Conversation

ZolAnder85
Copy link

Description:

I was trying to achieve a non-overlapping scrollbar and rounded, non-touching scrollbar-thumbs.
I ended up coding it so I can use it.
I think – since I have already done it – it is worth adding as it could only be achieved by rewriting the whole scroller source.
With the default theme values I added, results should look identical.

I don't know what version you would be adding this, so please add the since comments.
I hope this still counts as a "small contribution" :)

Implements #4360 too, if you tweak the default theme.

Here is an image what I was looking for:

Demo

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for the contribution. Having rounded scrollbars is a really nice change. There are a few things I'd like to changed here so please see below:

  • Per the PR template, we do all of the feature development on the develop branch. Would you mind targeting that instead of master?

  • Instead of just making it possible to make the scrollbar rounded, can you make it rounded in the default theme? The idea with Scroll bar should have rounded corners, at least in its expanded state #4360 is of course to have that as the default after all.

  • Would you mind separating rounded scrollbar and padding into two separate PRs? I see that as two different changes and the latter might require a bit more discussions.

I don't know what version you would be adding this, so please add the since comments.

We only add bug fixes to 2.4.x (and other path releases) so please add Since: 2.5 lines.

Again, we really do appreciate the contribution. I hope my many requested changes don't put you off :)

@Jacalz
Copy link
Member

Jacalz commented Jan 29, 2024

Forgot a don't in the last sentance (edited that in afterwards). I don't mean to put you off is what I meant 😅

@ZolAnder85
Copy link
Author

I did understand it either way, don't worry. I have limited amount of time to spend on this, so it might take some time.
I added these as one PR, because I felt that corner rounding without the padding does not look right. I also did not want to decide to change default looks. But I am glad if you like it. Anyway, I hope I won't have to make you wait for too long.

Copy link
Member

@andydotxyz andydotxyz 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 like a nice improvement, but it seems like it could be done without 5 new size types and associated package methods.
One thing to note is that we have moved to theme.Size(...) to avoid API explosion so that cuts the API surface in half.

It is not immediately obvious what the 3 different padding values are - a padding to avoid overlap makes sense but the others don't so much. Additionally I don't think there is much value in making the "limit" customisable - we should always avoid them being too small for tapping.

@ZolAnder85
Copy link
Author

Sorry, but I have no time for this. If you want to add these changes, feel free to use the changes. I will keep the fork for now, but I will eventually remove it.

@andydotxyz
Copy link
Member

Sorry, but I have no time for this. If you want to add these changes, feel free to use the changes. I will keep the fork for now, but I will eventually remove it.

Sorry to hear that. If you have time later and would like to come back to it please do, and consider the comments left earlier.
Best of luck.

@andydotxyz andydotxyz closed this Apr 17, 2024
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

3 participants