-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[Feat] Stroke color for Slider value indicator #135986
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
Conversation
|
@838 Overall this looks great. We should've one test that checks the Slider using |
|
@TahaTesser The test is now added. |
TahaTesser
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! We'll need a second review from @Piinks.
Piinks
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.
Hi @838 welcome! Thank you for contributing.
|
@838 |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
TahaTesser
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.
Good job with the updates.
While reviewing your PR, I've created code sample to check all the shapes you've updated here
Code sample
expand to view the code sample
import 'package:flutter/material.dart';
void main() => runApp(const MyApp());
class MyApp extends StatelessWidget {
const MyApp({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
debugShowCheckedModeBanner: false,
theme: ThemeData.dark(useMaterial3: true),
home: const Example(),
);
}
}
class Example extends StatefulWidget {
const Example({super.key});
@override
State<Example> createState() => _ExampleState();
}
class _ExampleState extends State<Example> {
double _sliderValue = 0.5;
RangeValues _rangeSliderValues = const RangeValues(0.25, 0.75);
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
title: const Text('Sliders'),
),
body: Column(
mainAxisAlignment: MainAxisAlignment.spaceEvenly,
children: [
_buildSlider(range: false),
_buildSlider(
range: false,
rectangularSliderValueIndicatorShape:
const RectangularSliderValueIndicatorShape(),
),
_buildSlider(
range: false,
paddleSliderValueIndicatorShape:
const PaddleSliderValueIndicatorShape(),
),
_buildSlider(
range: true,
rectangularRangeSliderValueIndicatorShape:
const RectangularRangeSliderValueIndicatorShape(),
),
_buildSlider(
range: true,
paddleRangeSliderValueIndicatorShape:
const PaddleRangeSliderValueIndicatorShape(),
),
],
),
);
}
Widget _buildSlider({
bool range = false,
RectangularSliderValueIndicatorShape? rectangularSliderValueIndicatorShape,
PaddleSliderValueIndicatorShape? paddleSliderValueIndicatorShape,
RectangularRangeSliderValueIndicatorShape? rectangularRangeSliderValueIndicatorShape,
PaddleRangeSliderValueIndicatorShape? paddleRangeSliderValueIndicatorShape,
}) {
if (range) {
return SliderTheme(
data: SliderThemeData(
rangeValueIndicatorShape: rectangularRangeSliderValueIndicatorShape ??
paddleRangeSliderValueIndicatorShape,
valueIndicatorColor: Theme.of(context).colorScheme.surface,
valueIndicatorStrokeColor: Theme.of(context).colorScheme.tertiary,
showValueIndicator: ShowValueIndicator.always,
valueIndicatorTextStyle: TextStyle(
color: Theme.of(context).colorScheme.onSurface,
),
),
child: RangeSlider(
values: _rangeSliderValues,
onChanged: (value) {
setState(() {
_rangeSliderValues = value;
});
},
labels: RangeLabels(
_rangeSliderValues.start.round().toString(),
_rangeSliderValues.end.round().toString(),
),
min: 0,
max: 1,
),
);
}
return SliderTheme(
data: SliderThemeData(
valueIndicatorShape: rectangularSliderValueIndicatorShape ??
paddleSliderValueIndicatorShape,
valueIndicatorColor: Theme.of(context).colorScheme.surface,
valueIndicatorStrokeColor: Theme.of(context).colorScheme.tertiary,
showValueIndicator: ShowValueIndicator.always,
valueIndicatorTextStyle: TextStyle(
color: Theme.of(context).colorScheme.onSurface,
),
),
child: Slider(
value: _sliderValue,
onChanged: (value) {
setState(() {
_sliderValue = value;
});
},
label: _sliderValue.round().toString(),
min: 0,
max: 1,
),
);
}
}| sizeWithOverflow: sizeWithOverflow!, | ||
| backgroundPaintColor: sliderTheme!.valueIndicatorColor!, | ||
| strokePaintColor: isOnTop! ? sliderTheme.overlappingShapeStrokeColor : null, | ||
| strokePaintColor: isOnTop! ? sliderTheme.overlappingShapeStrokeColor : sliderTheme.valueIndicatorStrokeColor, |
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 like you added tests for all the value indicator shapes except this RectangularRangeSliderValueIndicatorShape
| ..path(color: theme.sliderTheme.valueIndicatorColor), | ||
| ); | ||
| }); | ||
|
|
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.
RectangularRangeSliderValueIndicatorShape test here
|
I appreciate your input Should I also add tests to verify that 'overlappingShapeStrokeColor' behaves correctly when a |
|
Yes, in the same test class |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| await gesture.up(); | ||
| }); | ||
|
|
||
| testWidgetsWithLeakTracking('RectangularRangeSliderValueIndicatorShape supports SliderTheme.valueIndicatorStrokeColor on overlapping indicator', (WidgetTester tester) async { |
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.
Is this a duplicate 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.
Not entirely duplicated.
In the scenario where RectangularRangeSliderValueIndicatorShape supports SliderTheme.valueIndicatorStrokeColor, the indicator values are set as 0 and 0.5 to ensure they do not overlap, and the overlappingShapeStrokeColor is left unset in SliderTheme.
However, in this particular test case, both indicator values are configured as 0, and the overlappingShapeStrokeColor is explicitly defined in SliderTheme to ensure overlappingShapeStrokeColor is used instead of the valueIndicatorStrokeColor
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.
Good idea
| await gesture.up(); | ||
| }); | ||
|
|
||
| testWidgetsWithLeakTracking('PaddleRangeSliderValueIndicatorShape supports SliderTheme.valueIndicatorStrokeColor on overlapping indicator', (WidgetTester tester) async { |
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 looks like a duplicate 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.
Same as this comment but instead testing PaddleRangeSliderValueIndicatorShape
TahaTesser
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!
We'll need a second review before we land this.
|
Goldens are approved! |
Piinks
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.
Manual roll Flutter from 6eea6e2 to 189196d (44 revisions) Manual roll requested by [email protected] flutter/flutter@6eea6e2...189196d 2023-10-18 [email protected] Roll Flutter Engine from ab86c53c19cd to 6caee3236d37 (2 revisions) (flutter/flutter#136834) 2023-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Reland] Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136839) 2023-10-18 [email protected] Convert menus to use OverlayPortal (flutter/flutter#130534) 2023-10-18 [email protected] [Reland] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136751) 2023-10-18 [email protected] Roll Flutter Engine from 0eff5d191856 to ab86c53c19cd (1 revision) (flutter/flutter#136832) 2023-10-18 [email protected] Fixes ability to call nextFocus() on a node to focus its descendant (flutter/flutter#136773) 2023-10-18 [email protected] Roll Flutter Engine from b778a07f8ae9 to 0eff5d191856 (6 revisions) (flutter/flutter#136829) 2023-10-18 [email protected] Implement GApplication:shutdown so a Flutter developer knows where to put code that should occur on application shutdown. (flutter/flutter#136780) 2023-10-18 [email protected] Roll Flutter Engine from 7f37c9b181af to b778a07f8ae9 (1 revision) (flutter/flutter#136818) 2023-10-18 [email protected] Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on `SliderInteraction.slideOnly` allowed interaction (flutter/flutter#136720) 2023-10-18 [email protected] Roll Flutter Engine from 78026b4003fe to 7f37c9b181af (1 revision) (flutter/flutter#136817) 2023-10-18 [email protected] [Feat] Stroke color for Slider value indicator (flutter/flutter#135986) 2023-10-18 [email protected] Fixed : Empty Rows shown at last page in Paginated data table (flutter/flutter#132646) 2023-10-18 [email protected] Roll Flutter Engine from 46923fd39032 to 78026b4003fe (1 revision) (flutter/flutter#136814) 2023-10-18 [email protected] Roll Packages from d439062 to 14aa69e (1 revision) (flutter/flutter#136813) 2023-10-18 [email protected] Don't build native assets in `flutter build bundle` (flutter/flutter#136641) 2023-10-18 [email protected] Roll Flutter Engine from c9c9684e03a3 to 46923fd39032 (1 revision) (flutter/flutter#136795) 2023-10-18 [email protected] Roll Flutter Engine from 0c1c29271e8b to c9c9684e03a3 (1 revision) (flutter/flutter#136792) 2023-10-18 [email protected] Roll Flutter Engine from 1de09d13e708 to 0c1c29271e8b (3 revisions) (flutter/flutter#136789) 2023-10-18 [email protected] Roll Flutter Engine from 6fc36e61a99a to 1de09d13e708 (1 revision) (flutter/flutter#136785) 2023-10-18 [email protected] Roll Flutter Engine from 3f818efff3c5 to 6fc36e61a99a (1 revision) (flutter/flutter#136781) 2023-10-18 [email protected] Roll Flutter Engine from 2eef9b43cfb4 to 3f818efff3c5 (3 revisions) (flutter/flutter#136777) 2023-10-18 [email protected] Roll Flutter Engine from 5df7af34a718 to 2eef9b43cfb4 (4 revisions) (flutter/flutter#136774) 2023-10-18 [email protected] Flutter preview device (flutter/flutter#135639) 2023-10-18 [email protected] Add findChildIndexCallback examples (flutter/flutter#133469) 2023-10-18 [email protected] Adds API for performing semantics actions in tests (flutter/flutter#132598) 2023-10-18 [email protected] Roll Flutter Engine from e57b5bac4244 to 5df7af34a718 (2 revisions) (flutter/flutter#136772) 2023-10-17 [email protected] Roll Flutter Engine from f9f937e51080 to e57b5bac4244 (3 revisions) (flutter/flutter#136768) 2023-10-17 [email protected] clean up `--dart-define-from-file` option tests (flutter/flutter#135980) 2023-10-17 [email protected] GoldenFileComparators should dispose created Image objects. (flutter/flutter#136716) 2023-10-17 [email protected] make integration_test_driver_extended.dart support writeResponseData--(done) (flutter/flutter#128382) 2023-10-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.1.1 (flutter/flutter#136762) 2023-10-17 [email protected] Roll Flutter Engine from 289f29b1ad00 to f9f937e51080 (1 revision) (flutter/flutter#136755) 2023-10-17 [email protected] Roll Flutter Engine from 659e68a097b5 to 289f29b1ad00 (1 revision) (flutter/flutter#136752) 2023-10-17 [email protected] Support --web-header option for flutter run (flutter/flutter#136297) 2023-10-17 [email protected] Add Android 14 physical devices to firebase tests (flutter/flutter#136736) 2023-10-17 [email protected] Roll Flutter Engine from 62a90a91cee3 to 659e68a097b5 (3 revisions) (flutter/flutter#136746) 2023-10-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136750) 2023-10-17 [email protected] Revert "[SingleChildScrollView] Correct the offset pixels if it is out of range during layout" (flutter/flutter#136744) 2023-10-17 [email protected] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136562) 2023-10-17 [email protected] Roll Flutter Engine from 3ecbe924a598 to 62a90a91cee3 (2 revisions) (flutter/flutter#136739) 2023-10-17 [email protected] Roll Flutter Engine from 0a4d8b99a95b to 3ecbe924a598 (3 revisions) (flutter/flutter#136732) 2023-10-17 [email protected] Reenable NexusLowRes API 29 (flutter/flutter#136686) 2023-10-17 [email protected] Reenable the nexus 6p tests (flutter/flutter#136689) ...

Consider a scenario where the background color and indicator's background color are the same.
Adding a stroke color to the value indicator would be a valuable for the following reasons:
List which issues are fixed by this PR. You must list at least one issue.
Fixes #135984
Pre-launch Checklist
///).