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

Introduce new Slider shapes #147783

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Introduce new Slider shapes #147783

wants to merge 2 commits into from
+1,979 −61

Conversation

TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented May 3, 2024 •
edited

fixes Update Slider for Material 3 redesign

Description

This PR adds new Slider shapes BarSliderThumbShape, ExpressiveRoundedRectSliderTrackShape, and RoundedRectSliderValueIndicatorShape.

When these shapes are applied in the SliderTheme, the sliders uses the new Material Design Slider elements.

    final ThemeData themeData = ThemeData(
      sliderTheme: const SliderThemeData(
        thumbShape: BarSliderThumbShape(),
        trackShape: ExpressiveRoundedRectSliderTrackShape(),
        valueIndicatorShape: RoundedRectSliderValueIndicatorShape(),
      ),
    );

Preview

BarSliderThumbShape only draws thumb overlay in dark mode.

Light mode Dark mode
Thumb Pressed Thumb

New shapes when dragged

Light mode Dark mode
Record_2024-05-03-17-12-14.mp4
Record_2024-05-03-17-12-28.mp4

Stop indicator in the RoundedRectSliderValueIndicatorShape track shape

Preview 1 Preview 2
stop_indicator.mov
trackGapSize set to 0 Overridden trackGapSize & barThumbSize

Pre-launch Checklist

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: material design flutter/packages/flutter/material repository. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels May 3, 2024
@TahaTesser TahaTesser marked this pull request as ready for review May 3, 2024 15:42
@TahaTesser TahaTesser requested a review from Piinks May 3, 2024 17:13
guidezpl
guidezpl reviewed May 6, 2024
View reviewed changes
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

Nice work, just a general comment, I think we should make it easier to use the new shapes, since they will become the new default. Rather than manually specifying them, developers could flip a flag on ThemeData.

packages/flutter/lib/src/material/slider.dart
Size? get barThumbSize => const Size(4.0, 44.0);

@override
double? get trackGapSize => 6.0;
}

// TODO(quncheng): Update M3 defaults to match the latest specs.
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this TODO?

@TahaTesser
Copy link
Member Author

Nice work, just a general comment, I think we should make it easier to use the new shapes, since they will become the new default. Rather than manually specifying them, developers could flip a flag on ThemeData.

Interesting idea, tho this is only specific to Slider (next RangeSlider). Maybe in the SliderThemeData? Do you've a name for this flag?

@guidezpl
Copy link
Member

guidezpl commented May 6, 2024

Perhaps useUpdatedSliderShapes or use2024SliderShapes? I think it would apply to all types of sliders

@TahaTesser
Copy link
Member Author

Perhaps useUpdatedSliderShapes or use2024SliderShapes? I think it would apply to all types of sliders

Added use2024SliderShapes flag, updated defaults and tests. I'll push the changes with updated docs tomorrow.

@TahaTesser TahaTesser requested a review from guidezpl May 8, 2024 15:03
guidezpl
guidezpl requested changes May 10, 2024
View reviewed changes
packages/flutter/lib/src/material/slider.dart
Comment on lines +794 to +809
final SliderThemeData defaultsM3 = _SliderDefaultsM3(
context: context,
sliderTheme: sliderTheme.copyWith(
trackShape: trackShape,
thumbShape: thumbShape,
valueIndicatorShape: valueIndicatorShape,
),
);
final SliderThemeData defaultsM2 = _SliderDefaultsM2(
context: context,
sliderTheme: sliderTheme.copyWith(
trackShape: trackShape,
thumbShape: thumbShape,
valueIndicatorShape: valueIndicatorShape,
),
);
Copy link
Member

Choose a reason for hiding this comment

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

let's avoid creating both defaults every time since we know we'll only use one

packages/flutter/lib/src/material/slider_theme.dart
@@ -3536,3 +3601,413 @@ class _DropSliderValueIndicatorPathPainter {
canvas.restore();
}
}

Copy link
Member

Choose a reason for hiding this comment

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

for each of these new shapes, please provide context about when they were introduced, and mention the new flag

packages/flutter/lib/src/material/slider_theme.dart
/// A temporary flag that can be used to opt-in to the new 2024 slider shapes.
///
/// This flag is _false_ by default. If true, then the [Slider]'s will use the
/// new 2024 slider shapes.
Copy link
Member

Choose a reason for hiding this comment

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

explicit that this flag will be defaulted to true in the future, and then deprecated

packages/flutter/lib/src/material/slider_theme.dart
/// * trackShape: [RoundedRectSliderTrackShape] is replaced with [ExpressiveRoundedRectSliderTrackShape].
/// * thumbShape: [RoundSliderThumbShape] is replaced with [BarSliderThumbShape].
/// * valueIndicatorShape: [PaddleRangeSliderValueIndicatorShape] is replaced with [RoundedRectSliderValueIndicatorShape].
final bool? use2024SliderShapes;
Copy link
Member

Choose a reason for hiding this comment

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

please link to code sample

packages/flutter/lib/src/material/slider_theme.dart
Comment on lines +629 to +632
/// This flag effects the following shapes if [ThemeData.useMaterial3] is set to false:
/// * trackShape: [RoundedRectSliderTrackShape] is replaced with [ExpressiveRoundedRectSliderTrackShape].
/// * thumbShape: [RoundSliderThumbShape] is replaced with [BarSliderThumbShape].
/// * valueIndicatorShape: [PaddleRangeSliderValueIndicatorShape] is replaced with [RoundedRectSliderValueIndicatorShape].
Copy link
Member

@guidezpl guidezpl May 10, 2024 •
edited

Choose a reason for hiding this comment

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

I think this is overkill, in the sense that useMaterial3 and this flag both being true should not be a valid combination that we support

packages/flutter/lib/src/material/slider_theme.dart
/// * [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 ExpressiveRoundedRectSliderTrackShape extends SliderTrackShape with BaseSliderTrackShape {
Copy link
Member

Choose a reason for hiding this comment

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

is this the term used in guidelines?

packages/flutter/lib/src/material/slider.dart
tickMarkShape: sliderTheme.tickMarkShape ?? defaultTickMarkShape,
thumbShape: sliderTheme.thumbShape ?? defaultThumbShape,
overlayShape: sliderTheme.overlayShape ?? defaultOverlayShape,
trackShape: trackShape,
Copy link
Member

@guidezpl guidezpl May 10, 2024 •
edited

Choose a reason for hiding this comment

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

for consistency with other properties, please keep the ?? defaults.X here

packages/flutter/lib/src/material/slider.dart
overlayShape: sliderTheme.overlayShape ?? defaultOverlayShape,
trackShape: trackShape,
tickMarkShape: sliderTheme.tickMarkShape ?? defaults.tickMarkShape,
thumbShape: thumbShape,
Copy link
Member

@guidezpl guidezpl May 10, 2024 •
edited

Choose a reason for hiding this comment

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

for consistency with other properties, please keep the ?? defaults.X here

packages/flutter/lib/src/material/slider.dart
Comment on lines +785 to +793
final bool newShapes = sliderTheme.use2024SliderShapes ?? false;
final SliderTrackShape trackShape = sliderTheme.trackShape ??
(newShapes ? const ExpressiveRoundedRectSliderTrackShape() as SliderTrackShape : const RoundedRectSliderTrackShape());
final SliderComponentShape thumbShape = sliderTheme.thumbShape ??
(newShapes ? const BarSliderThumbShape() : const RoundSliderThumbShape());
final SliderComponentShape valueIndicatorShape = sliderTheme.valueIndicatorShape ??
(newShapes
? const RoundedRectSliderValueIndicatorShape()
: theme.useMaterial3 ? const DropSliderValueIndicatorShape() : const RectangularSliderValueIndicatorShape());
Copy link
Member

@guidezpl guidezpl May 10, 2024 •
edited

Choose a reason for hiding this comment

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

shouldn't this code be in the defaults and computed once, rather than every build, here?

dev/tools/gen_defaults/lib/slider_template.dart
SliderComponentShape? get valueIndicatorShape => const DropSliderValueIndicatorShape();
Color? get valueIndicatorColor {
if (_sliderTheme.valueIndicatorShape is RoundedRectSliderValueIndicatorShape) {
return _colors.inverseSurface;
Copy link
Member

Choose a reason for hiding this comment

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

please add a TODO to use tokens

@TahaTesser
Copy link
Member Author

@guidezpl
Appreciate the review! I'm about start traveling for Google I/O 2024. I'll address them after returning from states.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guidezpl guidezpl guidezpl requested changes

@Piinks Piinks Awaiting requested review from Piinks

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. 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.

Update Slider for Material 3 redesign
2 participants
@TahaTesser @guidezpl