Skip to content

Use ScrollbarTheme instead Theme for Scrollbar #113237

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 1 commit into from
Oct 19, 2022
Merged

Use ScrollbarTheme instead Theme for Scrollbar #113237

auto-submit merged 1 commit into from
Oct 19, 2022

Conversation

@Oleh-Sv
Copy link
Contributor

@Oleh-Sv Oleh-Sv commented Oct 10, 2022 •
edited by xu-baolin
Loading

When we are using Theme directly scrollbar ignores ScrollbarTheme widgets in tree. We should use ScrollbarTheme.of(context) for get correct theme. Implementation of ScrollbarTheme.of(context) already use Theme.of(context) if we don't have any ScrollbarTheme widgets in tree.

Issue
Fixes #113235

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
Copy link

flutter-dashboard bot commented Oct 10, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Oct 10, 2022
@TahaTesser
Copy link
Member

TahaTesser commented Oct 12, 2022

@Oleh-Sv
Thanks for fixing it, this needs a test in order to land.

@Oleh-Sv
Copy link
Contributor Author

Oleh-Sv commented Oct 12, 2022

@TahaTesser Test is added

xu-baolin
xu-baolin reviewed Oct 13, 2022
View reviewed changes
packages/flutter/test/material/scrollbar_theme_test.dart Outdated
Copy link
Member

@xu-baolin xu-baolin Oct 13, 2022

Choose a reason for hiding this comment

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

Use thumbVisibility instead.

Copy link
Member

@xu-baolin xu-baolin Oct 13, 2022

Choose a reason for hiding this comment

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

Looks good with a small nit.

Copy link
Contributor Author

@Oleh-Sv Oleh-Sv Oct 16, 2022

Choose a reason for hiding this comment

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

@xu-baolin thanks for comment. Ready

@Oleh-Sv Oleh-Sv closed this Oct 16, 2022
@Oleh-Sv Oleh-Sv reopened this Oct 16, 2022
xu-baolin
xu-baolin approved these changes Oct 16, 2022
View reviewed changes
Copy link
Member

@xu-baolin xu-baolin 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

@xu-baolin xu-baolin added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 16, 2022

auto label is removed for flutter/flutter, pr: 113237, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 16, 2022

auto label is removed for flutter/flutter, pr: 113237, due to Validations Fail.

@Oleh-Sv
Copy link
Contributor Author

Oleh-Sv commented Oct 18, 2022

@Piinks @TahaTesser Can someone complete review and give second approve?

TahaTesser
TahaTesser approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM!

@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 19, 2022
@auto-submit auto-submit bot merged commit abfaec6 into flutter:master Oct 19, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 19, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 19, 2022
@engine-flutter-autoroll
abfaec6 Use ScrollbarTheme instead Theme for Scrollbar (flutter/flutt…
685f4cc 
…er#113237)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 19, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 19, 2022
@engine-flutter-autoroll
abfaec6 Use ScrollbarTheme instead Theme for Scrollbar (flutter/flutt…
bc63a10 
…er#113237)
@moffatman moffatman mentioned this pull request Jan 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TahaTesser TahaTesser TahaTesser approved these changes

@Piinks Piinks Awaiting requested review from Piinks

@xu-baolin xu-baolin Awaiting requested review from xu-baolin

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Scrollbar ignores ScrollbarTheme widget and always directly use ScrollbarThemeData from Theme widget

3 participants

@Oleh-Sv @TahaTesser @xu-baolin