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.

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

Improve some scrollbar error messages #143279

Merged
merged 7 commits into from
Feb 13, 2024
Merged

Improve some scrollbar error messages #143279

merged 7 commits into from
Feb 13, 2024

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Feb 11, 2024 •
edited
Loading

Adds some missing spaces, rewords some errors, and splits some errors into more lines.

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.

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Feb 11, 2024
@loic-sharma
Copy link
Member Author

@Piinks It looks like the existing unit tests don't assert on the error hints. Should I add tests just for this case? Or should I get a test exception?

@goderbauer
Copy link
Member

Fly-by comment: I would add a test for this because I think if we would have had a test we would have realized that it is missing spaces earlier (e.g. at the latest during code review of the change that introduced the error).

Piinks
Piinks reviewed Feb 12, 2024
View reviewed changes
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.

+1 to Michael's comment. I feel like this error message has changed a lot over the years, so a test is a great idea. There is already a test that verifies it throws the error, it just does not check the whole message:

flutter/packages/flutter/test/widgets/scrollbar_test.dart

Line 1478 in 52f923c

testWidgets('Interactive scrollbars should have a valid scroll controller', (WidgetTester tester) async {

loic-sharma
loic-sharma commented Feb 12, 2024
View reviewed changes
packages/flutter/lib/src/widgets/scrollbar.dart
ErrorHint(
'If a ScrollController has not been provided, the '
'PrimaryScrollController is used by default on mobile platforms '
'for ScrollViews with an Axis.vertical scroll direction.',
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 updated this phrasing to match the corresponding hint in the "multiple scroll positions" assertion.

loic-sharma
loic-sharma commented Feb 12, 2024
View reviewed changes
packages/flutter/lib/src/widgets/scrollbar.dart
),
if (tryPrimary) ...<ErrorHint>[
Copy link
Member Author

@loic-sharma loic-sharma Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The previous error hint was one long line. I split this into multiple hints to split it into multiple lines. Please let me know if I should revert this split.

loic-sharma
loic-sharma commented Feb 12, 2024
View reviewed changes
packages/flutter/lib/src/widgets/scrollbar.dart
else
ErrorHint(
'The provided ScrollController cannot be shared by multiple '
'ScrollView widgets.'
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 changed this hint as well

@loic-sharma loic-sharma changed the title Add missing spaces in scroll errors Improve some scrollbar error messages Feb 12, 2024
@loic-sharma loic-sharma marked this pull request as ready for review February 12, 2024 23:28
@loic-sharma
Copy link
Member Author

Thanks all for the feedback! This should be ready for review now. Let me know if I goofed anything, I'm a massive framework newbie 😅

@github-actions github-actions bot added the f: material design flutter/packages/flutter/material repository. label Feb 13, 2024
@Piinks Piinks added the a: error message Error messages from the Flutter framework label Feb 13, 2024
Piinks
Piinks approved these changes Feb 13, 2024
View reviewed changes
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.

This is a really nice clean up. The error message had gotten so complex as we added more test cases over time. Thank you! LGTM

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 13, 2024
@auto-submit auto-submit bot merged commit b4270f7 into flutter:master Feb 13, 2024
67 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
f19b0e7
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
e58aec5
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
6445844
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
540b5f1
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
2ab98a0
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
c1fb725
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
0d4842b
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
41cdf9b
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
@engine-flutter-autoroll
b4270f7b0 Improve some scrollbar error messages (flutter/flutter#143279)
cbdca21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piinks Piinks Piinks approved these changes

Assignees
No one assigned
Labels
a: error message Error messages from the Flutter framework 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.

None yet
3 participants
@loic-sharma @goderbauer @Piinks