Skip to content

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Jun 17, 2021

This adjusts the assertions on interactive scrollbars to only be applied when the scrollbar is visible.

Related issues:

This also adds more documentation and code samples. I cleaned up a bit, removing outstanding todos and fixing some abbreviations in the tests.

For some of the removed todos, I filed issues to actually track the feature requests. 🙃

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 feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@Piinks Piinks added a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation a: error message Error messages from the Flutter framework c: tech-debt Technical debt, code quality, testing, etc. labels Jun 17, 2021
@flutter-dashboard flutter-dashboard bot added the f: cupertino flutter/packages/flutter/cupertino repository label Jun 17, 2021
@google-cla google-cla bot added the cla: yes label Jun 17, 2021
@Piinks Piinks requested a review from dnfield June 17, 2021 21:05
@davidmartos96
Copy link
Contributor

davidmartos96 commented Jun 18, 2021

I like the proposed changes. Delaying the assertion to when the scrollbar has been interacted with, will help a lot to tell which scrollbar in the tree is incorrect. The error in the gallery project was a bit confusing.

@Piinks Piinks requested a review from dnfield June 21, 2021 23:38
@Piinks
Copy link
Contributor Author

Piinks commented Jun 21, 2021

Delaying the assertion to when the scrollbar has been interacted with, will help a lot to tell which scrollbar in the tree is incorrect.

This should only delay it until it is visible and available for interaction. :)

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Thanks for the example, really helpful

Lgtm

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux flutter_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite analyze-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: error message Error messages from the Flutter framework a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. c: tech-debt Technical debt, code quality, testing, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository 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.

Scrollbar throws due to recent assert changes
4 participants