-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[Material] Unit test for skipping Slider tick mark due to overdensity #28013
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
[Material] Unit test for skipping Slider tick mark due to overdensity #28013
Conversation
rami-a
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
johnsonmh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
HansMuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some small stuff
|
|
||
| Widget buildSlider({ | ||
| int divisions, | ||
| TextDirection textDirection, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter isn't needed
| max: 100.0, | ||
| divisions: divisions, | ||
| value: value, | ||
| onChanged: (double newValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify for this test, since we're not moving the slider:
onChanged: (double value) { },
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| }); | ||
|
|
||
| testWidgets('Tick marks are skipped when they are too dense', (WidgetTester tester) async { | ||
| double value = 25.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed for this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| ), | ||
| ); | ||
|
|
||
| // No tick marks because they are too dense, just a thumb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failure mode we've seen has divisions = max - min (100). Would be best to duplicate that here to make sure we're not regressing a case that we've seen fail. We can assume that if 100 is too dense 500 will be too :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and added comment
| }) { | ||
| return Directionality( | ||
| textDirection: textDirection, | ||
| child: StatefulBuilder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need a StatefulBuilder, since we don't actually need to maintain the slider's value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
HansMuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adding a unit test for skipping over dense tick marks and updating the previous hot fix with better style.