Feat: Add carousel view theme by rkishan516 · Pull Request #164769 · flutter/flutter

rkishan516 · 2025-03-07T06:44:33Z

Feat: Add carousel view theme
fixes: #159679

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].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

navaronbracke · 2025-03-07T20:43:16Z

packages/flutter/lib/src/material/carousel.dart

-    final double effectiveElevation = widget.elevation ?? 0.0;
+        widget.backgroundColor ??
+        carouselTheme.backgroundColor ??
+        Theme.of(context).colorScheme.surface;


Suggested change

Theme.of(context).colorScheme.surface;

ColorScheme.of(context).surface;

navaronbracke · 2025-03-07T20:43:54Z

packages/flutter/lib/src/material/carousel.dart

+        carouselTheme.shape ??
        const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(28.0)));
    final WidgetStateProperty<Color?> effectiveOverlayColor =
+        widget.overlayColor ??


This line is duplicated? You probably mean to use the CarouselTheme's overlay color.

QuncCccccc

Thanks for your contribution:) I left some comments below. Please let me know if there's any questions.

@navaronbracke sorry I think I accidentally removed your review request somehow. Feel free review it!

QuncCccccc · 2025-04-09T17:01:05Z

packages/flutter/lib/src/material/carousel_theme.dart

+/// Defines default property values for descendant [CarouselView] widgets.
+///
+/// Descendant widgets obtain the current [CarouselViewThemeData] object using
+/// `CarouselTheme.of(context)`. Instances of [CarouselViewThemeData] can be


Suggested change

/// `CarouselTheme.of(context)`. Instances of [CarouselViewThemeData] can be

/// `CarouselViewTheme.of(context)`. Instances of [CarouselViewThemeData] can be

QuncCccccc · 2025-04-09T17:07:31Z

packages/flutter/lib/src/material/carousel_theme.dart

+    this.padding,
+  });
+
+  /// The padding between the carousel's edge and its children.


This doc looks a bit misleading. I think padding is the amount of space to surround each carousel item with. We might also want to mention (like other component theme doc), "Overrides the default value for [CarouselView.padding]". Here and the properties below.

QuncCccccc · 2025-04-09T17:08:11Z

packages/flutter/lib/src/material/carousel_theme.dart

+
+  /// The background color for each carousel item.
+  ///
+  /// Defaults to [ColorScheme.surface].


We don't need to mention the default value in CarouselViewThemeData.

QuncCccccc · 2025-04-09T17:15:27Z

packages/flutter/lib/src/material/carousel_theme.dart

+  }
+}
+
+/// Applies a carousel theme to descendant carousel widgets.


Suggested change

/// Applies a carousel theme to descendant carousel widgets.

/// Applies a carousel theme to descendant [CarouselView] widgets.

QuncCccccc · 2025-04-09T17:18:55Z

packages/flutter/lib/src/material/theme_data.dart

    TimePickerThemeData? timePickerTheme,
    ToggleButtonsThemeData? toggleButtonsTheme,
    TooltipThemeData? tooltipTheme,
+    CarouselViewThemeData? carouselViewTheme,


This list should follow alphabetical order. Here and below.

QuncCccccc · 2025-04-09T17:26:40Z

packages/flutter/test/material/carousel_theme_test.dart

+    expect(inkWell.overlayColor, carouselViewTheme.overlayColor);
+  });
+
+  testWidgets('CarouselViewTheme overrides defaults', (WidgetTester tester) async {


This test is basically identical with the test above. I think we can just keep one.

QuncCccccc · 2025-04-09T17:30:34Z

packages/flutter/test/material/carousel_theme_test.dart

+    expect(inkWell.overlayColor, overlayColor);
+  });
+}
+


Can we also test local CarouselViewTheme can override Theme.carouselViewTheme?

navaronbracke · 2025-04-09T17:43:26Z

packages/flutter/lib/src/material/carousel_theme.dart

+  ///
+  /// Defines each item's [Material.shape].
+  ///
+  /// Defaults to a [RoundedRectangleBorder] with a circular corner radius


As mentioned in https://github.com/flutter/flutter/pull/164769/files#r2035791842 the default value can be omitted from the doc here and below.

navaronbracke · 2025-04-09T17:44:43Z

packages/flutter/test/material/carousel_theme_test.dart

+    expect(carouselViewTheme.shape, null);
+  });
+
+  testWidgets('Default DividerThemeData debugFillProperties', (WidgetTester tester) async {


Suggested change

testWidgets('Default DividerThemeData debugFillProperties', (WidgetTester tester) async {

testWidgets('Default CarouselViewThemeData debugFillProperties', (WidgetTester tester) async {

rkishan516 · 2025-04-10T01:02:37Z

@QuncCccccc and @navaronbracke, Thanks for review. I have implemented all the changes.

QuncCccccc

LGTM:) Thanks for the contribution! Once the comments are fixed, I'll ask for the second review!

QuncCccccc · 2025-04-11T18:24:14Z

packages/flutter/lib/src/material/carousel_theme.dart

+  /// The shape of the carousel item's [Material].
+  ///
+  /// Defines each item's [Material.shape].


These 2 lines basically explain the same thing, so we can maybe just keep one?

QuncCccccc · 2025-04-11T18:25:52Z

packages/flutter/lib/src/material/carousel_theme.dart

+  /// Overrides the default value for [CarouselView.backgroundColor].
+  final Color? backgroundColor;
+
+  /// The z-coordinate at which to place this carousel relative to its parent.


I think this is not accurate. The elevation is for each carousel item instead of the whole carousel view.

QuncCccccc · 2025-04-11T18:36:28Z

packages/flutter/test/material/theme_data_test.dart

      timePickerTheme: const TimePickerThemeData(backgroundColor: Colors.black),
      toggleButtonsTheme: const ToggleButtonsThemeData(textStyle: TextStyle(color: Colors.black)),
      tooltipTheme: const TooltipThemeData(height: 100),
+      carouselViewTheme: const CarouselViewThemeData(),


This should be alphabetical order:)

rkishan516 · 2025-04-12T00:57:41Z

LGTM:) Thanks for the contribution! Once the comments are fixed, I'll ask for the second review!

I have pushed all mentioned changes.

QuncCccccc

LGTM. Thank you for your contribution!

QuncCccccc · 2025-04-14T20:08:29Z

packages/flutter/test/material/theme_data_test.dart

      timePickerTheme: const TimePickerThemeData(backgroundColor: Colors.white),
      toggleButtonsTheme: const ToggleButtonsThemeData(textStyle: TextStyle(color: Colors.white)),
      tooltipTheme: const TooltipThemeData(height: 100),
+      carouselViewTheme: const CarouselViewThemeData(),


alphabetical order here as well.

QuncCccccc · 2025-04-14T20:08:46Z

packages/flutter/test/material/theme_data_test.dart

      'timePickerTheme',
      'toggleButtonsTheme',
      'tooltipTheme',
+      'carouselViewTheme',


justinmc

LGTM with nits 👍

justinmc · 2025-04-15T22:17:48Z

packages/flutter/lib/src/material/carousel_theme.dart

+  /// Returns the configuration [data] from the closest [CarouselViewTheme] ancestor.
+  /// If there is no ancestor, it returns [ThemeData.carouselViewTheme].


Add an empty line of /// between these two sentences.

justinmc · 2025-04-15T22:20:28Z

packages/flutter/test/material/carousel_theme_test.dart

+        ),
+      ),
+    );
+    await tester.pumpAndSettle();


Is this pumpAndSettle necessary? If so, would just a pump work? Same question for the above usages of pumpAndSettle.

Not a huge deal but I try to be strict about frame pumping where possible.

No, It's not required. I have removed and pushed the change.

MitchellGoodwin

LGTM!

flutter/flutter@aef4718...ecabb1a 2025-04-17 [email protected] Hide error on mDNS registration failure and print warning in flutter attach (flutter/flutter#166782) 2025-04-17 [email protected] Add support for language hints in TextFields (Android only) (flutter/flutter#165554) 2025-04-17 [email protected] Roll Skia from d8f13372549d to cc2b57434651 (1 revision) (flutter/flutter#167331) 2025-04-17 [email protected] Add DropdownMenu.restorationId (flutter/flutter#166684) 2025-04-17 [email protected] Roll Skia from 3d6165b70199 to d8f13372549d (14 revisions) (flutter/flutter#167325) 2025-04-17 [email protected] fix(CircularProgressIndicator.adaptive): strokeWidth default value (flutter/flutter#165370) 2025-04-17 [email protected] Remove unnecessary builder (flutter/flutter#166623) 2025-04-17 [email protected] Add a `gradle_errors.dart`entry for missing NDK source.properties file (flutter/flutter#167320) 2025-04-17 [email protected] Roll Skia from 6627deb65939 to 3d6165b70199 (1 revision) (flutter/flutter#167255) 2025-04-17 [email protected] Manual roll Dart SDK from 87965ab4864e to 992221a362ec (35 revisions) (flutter/flutter#167243) 2025-04-16 [email protected] Roll Fuchsia Linux SDK from 7bWzHwIPBTyU6R9nO... to m8Aln7fTF_8zy1V9N... (flutter/flutter#167312) 2025-04-16 [email protected] Don't throw on error for mDNS when searching for Dart VML url on core devices (flutter/flutter#167135) 2025-04-16 [email protected] Fix: Localization for non zero time in 24 hours format (flutter/flutter#164885) 2025-04-16 [email protected] Updated docstrings for TextureContents (flutter/flutter#167221) 2025-04-16 [email protected] Broke cyclical dependency with dlpath and typographer. (flutter/flutter#167293) 2025-04-16 [email protected] Set an additional CMake arg in `forceNdkDownload` to make the task name align with the `BuildType` (flutter/flutter#167240) 2025-04-16 [email protected] Fix Carousel crashes when using PageStorageKey (flutter/flutter#166817) 2025-04-16 [email protected] Revert "Removed superfluous copy in license checker (#167146)" (flutter/flutter#167246) 2025-04-16 [email protected] [native assets] Roll dependencies (flutter/flutter#167287) 2025-04-16 [email protected] Marks Linux_pixel_7pro static_path_stroke_tessellation_perf__timeline_summary to be unflaky (flutter/flutter#167259) 2025-04-16 [email protected] Feat: Add carousel view theme (flutter/flutter#164769) 2025-04-16 [email protected] Marks Mac_ios dynamic_path_stroke_tessellation_perf_ios__timeline_summary to be unflaky (flutter/flutter#167261) 2025-04-16 [email protected] When using --local-web-sdk, use a locally built Dart SDK if one is available (flutter/flutter#166732) 2025-04-16 [email protected] Added emulator version to doctor (flutter/flutter#167236) 2025-04-16 [email protected] Marks Mac_ios static_path_stroke_tessellation_perf_ios__timeline_summary to be unflaky (flutter/flutter#167260) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Feat: Add carousel view theme fixes: flutter#159679 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.

flutter/flutter@aef4718...ecabb1a 2025-04-17 [email protected] Hide error on mDNS registration failure and print warning in flutter attach (flutter/flutter#166782) 2025-04-17 [email protected] Add support for language hints in TextFields (Android only) (flutter/flutter#165554) 2025-04-17 [email protected] Roll Skia from d8f13372549d to cc2b57434651 (1 revision) (flutter/flutter#167331) 2025-04-17 [email protected] Add DropdownMenu.restorationId (flutter/flutter#166684) 2025-04-17 [email protected] Roll Skia from 3d6165b70199 to d8f13372549d (14 revisions) (flutter/flutter#167325) 2025-04-17 [email protected] fix(CircularProgressIndicator.adaptive): strokeWidth default value (flutter/flutter#165370) 2025-04-17 [email protected] Remove unnecessary builder (flutter/flutter#166623) 2025-04-17 [email protected] Add a `gradle_errors.dart`entry for missing NDK source.properties file (flutter/flutter#167320) 2025-04-17 [email protected] Roll Skia from 6627deb65939 to 3d6165b70199 (1 revision) (flutter/flutter#167255) 2025-04-17 [email protected] Manual roll Dart SDK from 87965ab4864e to 992221a362ec (35 revisions) (flutter/flutter#167243) 2025-04-16 [email protected] Roll Fuchsia Linux SDK from 7bWzHwIPBTyU6R9nO... to m8Aln7fTF_8zy1V9N... (flutter/flutter#167312) 2025-04-16 [email protected] Don't throw on error for mDNS when searching for Dart VML url on core devices (flutter/flutter#167135) 2025-04-16 [email protected] Fix: Localization for non zero time in 24 hours format (flutter/flutter#164885) 2025-04-16 [email protected] Updated docstrings for TextureContents (flutter/flutter#167221) 2025-04-16 [email protected] Broke cyclical dependency with dlpath and typographer. (flutter/flutter#167293) 2025-04-16 [email protected] Set an additional CMake arg in `forceNdkDownload` to make the task name align with the `BuildType` (flutter/flutter#167240) 2025-04-16 [email protected] Fix Carousel crashes when using PageStorageKey (flutter/flutter#166817) 2025-04-16 [email protected] Revert "Removed superfluous copy in license checker (#167146)" (flutter/flutter#167246) 2025-04-16 [email protected] [native assets] Roll dependencies (flutter/flutter#167287) 2025-04-16 [email protected] Marks Linux_pixel_7pro static_path_stroke_tessellation_perf__timeline_summary to be unflaky (flutter/flutter#167259) 2025-04-16 [email protected] Feat: Add carousel view theme (flutter/flutter#164769) 2025-04-16 [email protected] Marks Mac_ios dynamic_path_stroke_tessellation_perf_ios__timeline_summary to be unflaky (flutter/flutter#167261) 2025-04-16 [email protected] When using --local-web-sdk, use a locally built Dart SDK if one is available (flutter/flutter#166732) 2025-04-16 [email protected] Added emulator version to doctor (flutter/flutter#167236) 2025-04-16 [email protected] Marks Mac_ios static_path_stroke_tessellation_perf_ios__timeline_summary to be unflaky (flutter/flutter#167260) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Feat: Add carousel view theme fixes: flutter#159679 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 7, 2025

rkishan516 force-pushed the carousel-view-theme branch from 6c97e25 to 6f294d0 Compare March 7, 2025 07:04

navaronbracke reviewed Mar 7, 2025

View reviewed changes

rkishan516 force-pushed the carousel-view-theme branch 2 times, most recently from 134c5f6 to 7fdbad4 Compare March 7, 2025 23:17

rkishan516 requested a review from navaronbracke March 7, 2025 23:20

rkishan516 force-pushed the carousel-view-theme branch from ce0963f to c30bd61 Compare March 11, 2025 03:15

QuncCccccc requested review from QuncCccccc and removed request for navaronbracke March 28, 2025 17:40

QuncCccccc reviewed Apr 9, 2025

View reviewed changes

navaronbracke reviewed Apr 9, 2025

View reviewed changes

rkishan516 force-pushed the carousel-view-theme branch from 033a4ce to b750224 Compare April 10, 2025 01:00

rkishan516 requested review from QuncCccccc and navaronbracke April 10, 2025 01:03

rkishan516 force-pushed the carousel-view-theme branch from b750224 to 9c0a09d Compare April 10, 2025 01:56

QuncCccccc reviewed Apr 11, 2025

View reviewed changes

rkishan516 force-pushed the carousel-view-theme branch from 9c0a09d to e471a37 Compare April 12, 2025 00:56

QuncCccccc approved these changes Apr 14, 2025

View reviewed changes

QuncCccccc reviewed Apr 14, 2025

View reviewed changes

rkishan516 force-pushed the carousel-view-theme branch from e471a37 to e4fbf8a Compare April 15, 2025 00:27

justinmc approved these changes Apr 15, 2025

View reviewed changes

MitchellGoodwin approved these changes Apr 15, 2025

View reviewed changes

rkishan516 force-pushed the carousel-view-theme branch from e4fbf8a to 7b4268e Compare April 16, 2025 00:15

Feat: Add carousel view theme

d61ebea

rkishan516 force-pushed the carousel-view-theme branch from 7b4268e to d61ebea Compare April 16, 2025 00:17

QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 16, 2025

auto-submit bot added this pull request to the merge queue Apr 16, 2025

Merged via the queue into flutter:master with commit ddc9540 Apr 16, 2025
75 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 16, 2025

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 17, 2025

ddc9540 Feat: Add carousel view theme (flutter/flutter#164769)

bce7de5

engine-flutter-autoroll mentioned this pull request Apr 17, 2025

Roll Flutter from aef4718b3956 to ecabb1a05212 (25 revisions) flutter/packages#9104

Merged

Piinks mentioned this pull request Apr 18, 2025

Flutter Notable Commits - 2025 #161460

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025

ddc9540 Feat: Add carousel view theme (flutter/flutter#164769)

e1d5d02

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025

ddc9540 Feat: Add carousel view theme (flutter/flutter#164769)

acea8fa

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025

ddc9540 Feat: Add carousel view theme (flutter/flutter#164769)

39aad64

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025

ddc9540 Feat: Add carousel view theme (flutter/flutter#164769)

0fdb8dd

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025

ddc9540 Feat: Add carousel view theme (flutter/flutter#164769)

bc2c19c

rkishan516 deleted the carousel-view-theme branch February 20, 2026 03:02

	Theme.of(context).colorScheme.surface;
	ColorScheme.of(context).surface;

	/// `CarouselTheme.of(context)`. Instances of [CarouselViewThemeData] can be
	/// `CarouselViewTheme.of(context)`. Instances of [CarouselViewThemeData] can be

	/// Applies a carousel theme to descendant carousel widgets.
	/// Applies a carousel theme to descendant [CarouselView] widgets.

	testWidgets('Default DividerThemeData debugFillProperties', (WidgetTester tester) async {
	testWidgets('Default CarouselViewThemeData debugFillProperties', (WidgetTester tester) async {

		/// Returns the configuration [data] from the closest [CarouselViewTheme] ancestor.
		/// If there is no ancestor, it returns [ThemeData.carouselViewTheme].

Conversation

rkishan516 commented Mar 7, 2025

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuncCccccc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented Apr 10, 2025

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented Apr 12, 2025

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

QuncCccccc left a comment •

edited

Loading