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.

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

[closed] Deprecate toggleableActiveColor #95870

Closed

Conversation

TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Dec 27, 2021

toggleableActiveColor is used in bunch of widgets where colorScheme is not adopted.
This PR deprecates toggleableActiveColor and updates all the widgets to use the colorScheme property.

  • Deprecated toggleableActiveColor and added changes for dart fix
  • Updated Switch, & SwitchListTile
  • Updated Checkbox & CheckboxListTitle
  • Updated Radio and RadioListTile
    ( had to expected colors in the test since Colors.blue[600] (default value of toggleableActiveColor is slightly different blue than colorScheme.secondar)

This is breaking as it would make toggleableActiveColor not have any effect on the above widgets

Related #90317 and #91772
Fixes #93709

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. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. c: tech-debt Technical debt, code quality, testing, etc. labels Dec 27, 2021
@skia-gold

This comment has been minimized.

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.

Thanks for doing this!

I believe this requires a (short) migration guide: https://github.com/flutter/flutter/wiki/Tree-hygiene#3-prepare-your-change.

Can you figure out which version to use for the deprecation? The wiki indicates the dev channel but now that it's deprecated, I'm not sure whether master or beta is more appropriate

packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/switch_list_tile.dart Outdated Show resolved Hide resolved
packages/flutter/lib/fix_data.yaml Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@TahaTesser

This comment has been minimized.

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.

I think the analysis error may be due to a missing period after the version

packages/flutter/lib/fix_data.yaml Outdated Show resolved Hide resolved
packages/flutter/lib/fix_data.yaml Outdated Show resolved Hide resolved
@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 22.
View them at https://flutter-gold.skia.org/cl/github/95870

@TahaTesser TahaTesser force-pushed the deprecate_toggleableActiveColor branch from e87fd19 to 4a76a03 Compare January 28, 2022 13:00
@flutter-dashboard
Copy link

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 package:flutter.

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

Changes reported for pull request #95870 at sha 4a76a03

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Jan 28, 2022
@TahaTesser
Copy link
Member Author

TahaTesser commented Jan 28, 2022

@guidezpl
Here is the update, the deprecation guide is live

Here are the things I did with this PR today

  1. Fix doc reference for ColorScheme.
  2. Updated the beta version in the deprecation message.
  3. Resolve fix_data.yaml, flutter/test_fixes/material.dart, flutter/test_fixes/material.dart.expect conflicts (due to another depracation PR)
  4. Updated dev/integration_tests/flutter_gallery/lib/gallery/themes.dart as it uses this color and it is deprecated and test was failing

Todo

After this lands, I will immediately file website PR to update timeline

Please let me know if there is anything else needed to land this, thank you!

@guidezpl
Copy link
Member

I'm working on pre-migrating internal uses so this won't be a breaking change internally, will report back when done

@guidezpl
Copy link
Member

guidezpl commented Feb 7, 2022

After some investigation, unfortunately, this won't be able to land without some prerequisite work. Many teams have defined a toggleableActiveColor which isn't ColorScheme.secondary and so that color must be defined via component themes.

However, RadioListTile, SwitchListTile and CheckboxListTile don't use the relevant component themes in the same way that Radio, Switch, and Checkbox do, when they were introduced in #70311. So first, the three list tile components will have to be updated to utilize component themes.

@TahaTesser
Copy link
Member Author

TahaTesser commented Feb 7, 2022

@guidezpl
Thanks so much for getting back.
Based on your feedback, I'm also updating RadioListTile, SwitchListTile and CheckboxListTile to support components theme.

These will be separate PRs of course :)

@rydmike
Copy link
Contributor

rydmike commented Feb 18, 2022

Just wanted to say @TahaTesser, awesome work on this topic and PR.

Looking forward to get this level of clarity and increased theming freedom on these widgets.

That toggleableActiveColor was used by all switches has been a long outstanding annoyance for me in Flutter theming, plus that the color is not set correctly e.g. by ThemeData.from(colorScheme: myScheme) or ThemeData(colorScheme: myScheme) to eg ColorScheme.secondary matching the most commonly used Material design intent, has been quite confusing to many devs.

This finally solves all those issues in one go, love it! 💙

@flutter-dashboard

This comment was marked as outdated.

@TahaTesser
Copy link
Member Author

Closing this for #97972

@TahaTesser TahaTesser closed this Apr 22, 2022
@TahaTesser TahaTesser deleted the deprecate_toggleableActiveColor branch April 22, 2022 10:15
@TahaTesser TahaTesser changed the title Deprecate toggleableActiveColor [closed] Deprecate toggleableActiveColor Aug 3, 2022
@sfshaza2
Copy link
Contributor

@TahaTesser, is this definitely landed in 3.7? I'd like to update the breaking changes page.

@TahaTesser
Copy link
Member Author

Hi @sfshaza2
Yes, I can confirm toggleableActiveColor deprecation is in the latest stable 3.7 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch widget color doesn't use ColorScheme
5 participants