Skip to content

Deprecate ThemeData.primaryX#110848

Closed
guidezpl wants to merge 15 commits intoflutter:masterfrom
guidezpl:deprecate-primaryX
Closed

Deprecate ThemeData.primaryX#110848
guidezpl wants to merge 15 commits intoflutter:masterfrom
guidezpl:deprecate-primaryX

Conversation

@guidezpl
Copy link
Member

@guidezpl guidezpl commented Sep 2, 2022 •
edited
Loading

In line with https://flutter.dev/go/material-theme-system-updates and #91772, deprecates redundant/unused ThemeData properties.

Old New Previous usages
ThemeData.primaryColor ColorScheme.primary too many to list
ThemeData.primaryColorLight removed CircleAvatar default background
ThemeData.primaryColorDart removed CircleAvatar default background

A Dart fix is provided.

This is not a breaking change, per our policy.

This PR also improves the description of ThemeData.colorScheme and re-orders a few already-deprecated ThemeData members to the right spot and fixes version number copy-paste error from #110162.

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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. c: tech-debt Technical debt, code quality, testing, etc. labels Sep 2, 2022
@guidezpl guidezpl mentioned this pull request Sep 2, 2022
Open
43 tasks
@guidezpl guidezpl mentioned this pull request Sep 9, 2022
Merged
11 tasks
@guidezpl guidezpl mentioned this pull request Sep 13, 2022
Merged
11 tasks
@guidezpl guidezpl marked this pull request as ready for review September 13, 2022 11:34
@guidezpl guidezpl requested a review from esouthren September 13, 2022 11:34
esouthren
esouthren reviewed Sep 13, 2022
View reviewed changes
esouthren
esouthren approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@esouthren esouthren left a comment

Choose a reason for hiding this comment

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

Great stuff! LGTM.

Piinks
Piinks requested changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

It looks like this broke a few things in flutter/plugins. Those will need to be migrated first before this can land.

@guidezpl
Copy link
Member Author

guidezpl commented Sep 14, 2022

Please see flutter/plugins#6417

@guidezpl
Copy link
Member Author

guidezpl commented Sep 15, 2022

@Piinks How does this look, especially the fixes?

Piinks
Piinks reviewed Sep 16, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Fixes look good. I still think this should be split in 2 at least. The clean up is great, but lumping it in with deprecating something means all of it could end up getting reverted if the deprecation breaks something internally. It is not uncommon for deprecations to get reverted.

Further, from the point of view of removing deprecations later, it is a lot easier if the git history is clean.

@guidezpl
undo fix
b0b3854 
undo fixes
@guidezpl
Copy link
Member Author

guidezpl commented Sep 16, 2022

That makes sense, I'll land the fixes in a separate PR.

Piinks
Piinks approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM once CI is happy. flutter/plugins probably just hasn't rolled in your change yet.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 6, 2022

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@guidezpl guidezpl closed this Oct 11, 2022
@rydmike rydmike mentioned this pull request Jan 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

@darrenaustin darrenaustin Awaiting requested review from darrenaustin

+1 more reviewer

@esouthren esouthren esouthren approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. c: tech-debt Technical debt, code quality, testing, etc. 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.

3 participants

@guidezpl @esouthren @Piinks