Skip to content

Conversation

@werainkhatri
Copy link
Member

Fixes #97876

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Feb 8, 2022
@werainkhatri werainkhatri force-pushed the trackradius-scrollbar branch from 5090253 to 01be575 Compare February 8, 2022 10:28
@werainkhatri
Copy link
Member Author

werainkhatri commented Feb 8, 2022

Should this PR add trackRadius to _MaterialScrollBar and CupertinoScrollBar too?

@goderbauer goderbauer requested a review from Piinks February 9, 2022 22:46
double crossAxisMargin = 0.0,
Radius? radius,
Radius? trackRadius,
OutlinedBorder? shape,
Copy link

Choose a reason for hiding this comment

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

I believe trackShape should also be added

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the need for trackShape @bdlukaa? It is not mentioned in #97876
I am cautious about adding more and more parameters to this widget.

Copy link

Choose a reason for hiding this comment

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

Just in case someone wants a very custom scrollbar. For my use case, only trackRadius should all, but, since in Flutter we should have control over every pixel on the screen, I believe trackShape is a good parameter for this widget.

We already have shape for the thumb, why not trackShape?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our engineering philosophy specifies

Avoid implementing features you don’t need.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#lazy-programming

Asking why not ...? is a slippery slope to a large and complicated API. Imagine a Scrollbar with 100 different parameters... 😵‍💫

You are right, in Flutter you can control every pixel on the screen, that does not mean we need to do so in the framework. You can certainly extend the scrollbar class in your own code, or even publish a package. There are a number of scrollbar packages on pub.dev with a variety of special features.

The framework focuses on providing fundamental core UI features, but anyone can change and modify as they like in their own project. You are very right that you can control every pixel as you desire. :)

Copy link

Choose a reason for hiding this comment

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

Indeed, it is not necessary for my use case. @werainkhatri what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

i agree @Piinks' point. we may add it in the future if a user has a specific use case. i'll go ahead and remove this change.

Copy link

Choose a reason for hiding this comment

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

Tests are now failing @werainkhatri

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for another contribution @werainkhatri!

@fluttergithubbot fluttergithubbot merged commit 281f142 into flutter:master Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Add Scrollbar trackRadius

4 participants