Skip to content

Removed ProgressIndicator accentColor dependency. #77976

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
Merged
fluttergithubbot merged 2 commits into from
Mar 13, 2021
Merged

Removed ProgressIndicator accentColor dependency. #77976

fluttergithubbot merged 2 commits into from
Mar 13, 2021

Conversation

@darrenaustin
Copy link
Contributor

@darrenaustin darrenaustin commented Mar 11, 2021

This PR removes the ProgressIndicator widget's accentColor dependency per #56918.

The previous implementation painted the color of the indicator with the value of the valueColor animated color. If that was null then it would default to the theme's ThemeData.accentColor. With this PR we are changing the default to the theme's colorScheme.primary. In addition we have added a new 'color' property to the indicator that makes it easier to set a constant color.

So to summarize, the color of the ProgressIndicator is now:

valueColor.value ??
color ??
theme.colorScheme.primary

We have also changed the backgroundColor to default to the theme's colorScheme.background instead of theme.backgroundColor. This is usually the same color, but will help with the transition to using the color scheme for everything.

Breaking change

This is a breaking change. For apps that do need a specific color for a progress indicator, you can use the color parameter on the indicator directly:

  CircularProgressIndicator(color: Colors.purple),

You can also of course set the value for the background color as well:

  LinearProgressIndicator(
    color: Colors.red,
    backgroundColor: Colors.blue,
  ),

A new tests were added to verify the default values for the indicators' color are now ColorScheme.primary.

This PR was tested against internal Google apps in cl/360773269

@darrenaustin
Removed ProgressIndicator accentColor dependency.
ee69773 
Added new 'color' property to ProgressIndicator to make it easier to specify a constant color for the indicator.
@darrenaustin
Added tests for the progress indicator's color.
5fc01d4
@darrenaustin darrenaustin added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 11, 2021
@darrenaustin darrenaustin requested a review from HansMuller March 11, 2021 23:45
@google-cla google-cla bot added the cla: yes label Mar 11, 2021
@darrenaustin darrenaustin mentioned this pull request Mar 11, 2021
Closed
19 tasks
@darrenaustin darrenaustin changed the title Accent progress indicator Removed ProgressIndicator accentColor dependency. Mar 12, 2021
HansMuller
HansMuller approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit fad7c36 into flutter:master Mar 13, 2021
@kf6gpe kf6gpe mentioned this pull request Mar 15, 2021
Closed
@davidmartos96
Copy link
Contributor

davidmartos96 commented Mar 24, 2021 •
edited
Loading

@HansMuller I've just encountered with this breaking change in the latest beta version. Since it's breaking (the primary color is being used instead of the secondary color), what is the path to take migration wise?

Are there any plans to create a ProgressIndicatorTheme or similar, or are we forced to create a custom progress indicator in case we want to keep using the accent color / secondary color?

@darrenaustin darrenaustin deleted the accent_progress_indicator branch March 24, 2021 23:57
@darrenaustin
Copy link
Contributor Author

darrenaustin commented Mar 25, 2021

We did discuss adding a theme for this, but it didn't seem like it would have affected that many apps. However, after hearing from you (and some internal users), I will put together a PR with a ProgressIndicatorTheme shortly. I will reference this bug so stay tuned.

@darrenaustin
Copy link
Contributor Author

darrenaustin commented Apr 23, 2021

Just put up a PR that adds a ProgressIndicatorTheme: #81075.

@Kavantix
Copy link
Contributor

Kavantix commented May 19, 2021

Just ran into this on stable, is there a reason you opted for using primary instead of secondary now?

It would also be nice if this breaking change could be added to the breaking changelog and announced on the flutter announce channel since I expect lots of people running into this change

@darrenaustin
Copy link
Contributor Author

darrenaustin commented May 19, 2021

Just ran into this on stable, is there a reason you opted for using primary instead of secondary now?

This was to match the Material Design spec which uses the primary color:

https://material.io/components/progress-indicators#usage

It would also be nice if this breaking change could be added to the breaking changelog and announced on the flutter announce channel since I expect lots of people running into this change

It will be added as part of a larger accent color deprecation update along with a migration guide.

Sorry if this caused an issue for you. We are trying to migrate a lot of these older top-level ThemeData params to using the color scheme instead, but we could have had better communication on this one.

@Mayb3Nots
Copy link
Contributor

Mayb3Nots commented May 24, 2021

Just ran into this on stable, is there a reason you opted for using primary instead of secondary now?

This was to match the Material Design spec which uses the primary color:

https://material.io/components/progress-indicators#usage

It would also be nice if this breaking change could be added to the breaking changelog and announced on the flutter announce channel since I expect lots of people running into this change

It will be added as part of a larger accent color deprecation update along with a migration guide.

Sorry if this caused an issue for you. We are trying to migrate a lot of these older top-level ThemeData params to using the color scheme instead, but we could have had better communication on this one.

@darrenaustin May I know where can I get this migration guide?

@scris scris mentioned this pull request Aug 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

6 participants

@darrenaustin @davidmartos96 @Kavantix @Mayb3Nots @HansMuller @fluttergithubbot