Introduce new Material 3 Slider shapes
#152237
Conversation
|
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. |
There was a problem hiding this comment.
Thanks for adding the feature! This looks so great! I haven't finish reviewing the whole change, will continue tomorrow😎!
I also noticed slider.0.dart and slider.1.dart are basically the same. I think slider.1.dart was added when useMaterial3 was still false? Maybe we can delete one of these and add a button to switch between M2 and M3 in just one example app. This can be done later in a separate PR:)
| /// If [SliderThemeData.thumbShape] is [BarSliderThumbShape], this property is used to | ||
| /// set the size of the thumb. Otherwise, the default thumb size is 4 pixels for the | ||
| /// width and 44 pixels for the height. | ||
| final MaterialStateProperty<Size?>? barThumbSize; |
There was a problem hiding this comment.
Would it be better if we just call it thumbSize? Just feel it would be more consistent with other properties.
There was a problem hiding this comment.
The reason i made it barThumbSize because it's a Size type and this is not compatible with existing Thumb as it is just a circle and it would be double aka radius value. If the user expect thumbSize to update existing thumb it might create confusion.
However, you raised a good point on consistency. I will take a look if thumbSize of Size type can be used for existing thumb radius by rounding to square value.
There was a problem hiding this comment.
I agree. I did not realize this theme data class was so large. There are a lot of parameters here, so having a clear name would be ideal.
There was a problem hiding this comment.
Maybe (in a separate change) we can look at this, and maybe group the parameters together based on the type of slider they apply to.
| Color? get disabledThumbColor => _colors.onSurface.withOpacity(0.38).withOpacity(0.38); | ||
|
|
||
| @override | ||
| Color? get overlayColor => MaterialStateColor.resolveWith((Set<MaterialState> states) { |
There was a problem hiding this comment.
From the specs, it seems we don't have overlayColor by default. Looks like these have been deprecated internally but haven't been updated on the website. Maybe we can change this to transparent color or change the default overlayShape to null?
There was a problem hiding this comment.
You're right, I also tested Slider on the native and it doesn't seem to have overlay color anymore but when using it there is no other way to indicate if the Slider is hovered or focused. I sent a Demo on Discord.
IMO, to avoid breaking existing tests and focus, hover behavior of the Flutter Slider widget, we can keep the overlay color as it is until we have other ways to indicate these states.
|
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. |
Piinks
left a comment
There was a problem hiding this comment.
Thank you for your patience here, I'll be much quicker on the follow up reviews. 🙏 💙
| labelPainter: _labelPainter, | ||
| parentBox: this, | ||
| sliderTheme: _sliderTheme, | ||
| sliderTheme: _sliderTheme.copyWith(barThumbSize: MaterialStatePropertyAll<Size>(Size(thumbWidth, thumbHeight))), |
| /// If [SliderThemeData.thumbShape] is [BarSliderThumbShape], this property is used to | ||
| /// set the size of the thumb. Otherwise, the default thumb size is 4 pixels for the | ||
| /// width and 44 pixels for the height. | ||
| final MaterialStateProperty<Size?>? barThumbSize; |
There was a problem hiding this comment.
I agree. I did not realize this theme data class was so large. There are a lot of parameters here, so having a clear name would be ideal.
| /// If [SliderThemeData.thumbShape] is [BarSliderThumbShape], this property is used to | ||
| /// set the size of the thumb. Otherwise, the default thumb size is 4 pixels for the | ||
| /// width and 44 pixels for the height. | ||
| final MaterialStateProperty<Size?>? barThumbSize; |
There was a problem hiding this comment.
Maybe (in a separate change) we can look at this, and maybe group the parameters together based on the type of slider they apply to.
| /// width and 44 pixels for the height. | ||
| final MaterialStateProperty<Size?>? barThumbSize; | ||
|
|
||
| /// The size of the gap between the active and inactive tracks of the [GappedSliderTrackShape]. |
There was a problem hiding this comment.
Is this only relevant for the M3 slider? Or both? We should check that the docs clearly state when/how a parameter/behavior/etc applies for M2 versus M3
There was a problem hiding this comment.
All the new shapes are compatible with M2/M3. Great suggestion
I added this documentation
/// The Slider defaults to [GappedSliderTrackShape] when the track shape is
/// not specified, and the [trackGapSize] can be used to adjust the gap size.
///
/// If the [ThemeData.useMaterial3] is false, then the Slider track shape
/// defaults to [RoundedRectSliderTrackShape] and the [trackGapSize] is ignored.
/// In this case, set the track shape to [GappedSliderTrackShape] to use the
/// [trackGapSize].
final double? trackGapSize;| } | ||
| } | ||
|
|
||
| /// The bar shape of a [Slider]'s thumb. |
There was a problem hiding this comment.
Another thought for another change later - there's a ton of stuff in this massive file. Might be worth sending a change after this to move everything that does not relate to slider theme out of this file to a new one.
There was a problem hiding this comment.
That's right, this file contains every Slider shape and it's messy.
|
Back from vacation, there are a lot of great feedback here and I'll address them. |
|
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. |
|
There were some conflicts which i resolved today. I'm addressing a few more API challenges such as supporting the new |
|
I have tried several ways to support the new flutter/packages/flutter/lib/src/material/slider_theme.dart Lines 2282 to 2289 in 99f00a1 To move forward, we might need to state the new |
|
@QuncCccccc @Piinks |
That SGTM. |
| } | ||
| } | ||
|
|
||
| Widget _buildMaterialSlider(BuildContext context) { |
There was a problem hiding this comment.
Should the docs on Slider be updated to reflect all of this new new? :)
There was a problem hiding this comment.
Bumping this, do we think there are any doc updates with this change?
There was a problem hiding this comment.
So sorry, i wanted to comment. Yes, I want to write this Slider doc along side the migration guide so i can mention the new stuff things in the migration guide and sync them with Slider docs. Migration guide will have steps to restore the previous Slider components
Now that PR is approved. I will initiate migration guide draft.
|
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. |
Piinks
left a comment
There was a problem hiding this comment.
LGTM, kicking off Google testing 🤞
Can you confirm the comment on Slider docs below?
| } | ||
| } | ||
|
|
||
| Widget _buildMaterialSlider(BuildContext context) { |
There was a problem hiding this comment.
Bumping this, do we think there are any doc updates with this change?
Slider designSlider shapes
| /// * [Slider], which includes an overlay defined by this shape. | ||
| /// * [SliderTheme], which can be used to configure the overlay shape of all | ||
| /// sliders in a widget subtree. | ||
| class BarSliderThumbShape extends SliderComponentShape { |
There was a problem hiding this comment.
This class name conflicts with one that exists in Google testing
There was a problem hiding this comment.
This was a name we invented in the previous PR with Pierre. Naming stuff is hard :D
Looking at this description. What If i call it HandleThumbShape or VerticalBarThumbShape?
Piinks
left a comment
There was a problem hiding this comment.
Let's fix the naming conflict and then I'll rerun the Google testing shard.
We might need to implement an opt in first, but it will be easier to evaluate that option once all the failures from the name conflict are gone.
I think we'll definitely want a migration guide for folks that do not want the new appearance.
I grabbed some of the screenshot test failures for you to review. Some of them looked unexpected. I can dig deeper on each in the next run, just wanted to see if any jumped out at you at first glance.
On the left is before, on the right is with this change.
|
@Piinks These look to be mostly because of the updated track height of 16 pixels. I wrote a migration guide I'll file it tomorrow after Bruno looks at it. This guide includes steps to use previous track height of 4 pixels. The tests you shared might need to adjust their track height to get rid of these differences. |
|
@Piinks I'm also holding on to new commits to update docs in this PR as you're checking out the tests differences in #152237 (review) |
|
Can you push a commit that resolves the name conflict first? There are many many failures, removing those from the name conflict would help a lot. Thanks! |
|
Hello, i am on the latest flutter, and i wanted to ask when these changes are online for everyone. flutter doktor: The Slider still look like the Material 2 one. but i want to use the one from Material 3 Kind regards |
In case you're still wondering: you need to set the |
|
Hi, I'm trying to use the latest Material 3 What is the correct way to use the latest Material 3 |
|
Hi! Setting |
|
@QuncCccccc but how do you use the latest M3 Slider? If I simply import the Slider component from flutter/lib/src/material it's still the old M2 design. So setting |
|
The design of M3 has changed a lot. Originally, the M3 Slider didn't change its style much except for the default colors, so I assume that's the "old M2 design" you're referring to. In late 2023, the M3 Slider design changed to its latest version. Because of this, we updated the Slider and introduced the year2023 flag to distinguish between M3 Sliders from before late 2023 and those from after. When we were migrating from M2 to M3, it caused many breaking changes, which was quite difficult for developers. For that reason, we decided to keep the "old" M3 Slider design as the default. We then introduced year2023 so developers could manually enable the flag to see the latest design while avoiding those breaking changes. So yes, setting year2023 to false is required if we wanted to see the latest Slider. |
|
Hi folks, old PRs are not the best venue for feedback and discussion. If there is an issue, please file one and we can follow up, including if we need to improve the documentation. |
fixes Update
Sliderfor Material 3 redesignprevious implementation #147783
Description
This PR introduces new Material 3 Slider design.
Slider Preview
Value indicator Preview
Screen.Recording.2024-07-24.at.16.39.16.mov
New stop indicator
Screen.Recording.2024-07-24.at.16.40.17.mov
Customized
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.