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

Update some material button docs #99019

Closed
wants to merge 5 commits into from
Closed

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Feb 23, 2022

Fixes #98692

Maybe fixes #98693

If this is a good improvement to MaterialButton, I can do the same to resolve #98691

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.

@Piinks Piinks added f: material design flutter/packages/flutter/material repository. d: api docs Issues with https://api.flutter.dev/ a: quality A truly polished experience documentation a: annoyance Repeatedly frustrating issues with non-experimental functionality labels Feb 23, 2022
@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 23, 2022
@@ -462,4 +453,5 @@ class MaterialButton extends StatelessWidget {
///
/// This mixin only exists to give the "label and icon" button widgets a distinct
/// type for the sake of [ButtonTheme].
// TODO(Piinks): Should this be deprecated?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only used by the buttons that are being removed, should it be deprecated @HansMuller? I checked customer references as well, no one is using this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@Piinks
Copy link
Contributor Author

Piinks commented Feb 23, 2022

@rydmike I would love to know what you think of this :)

@Piinks
Copy link
Contributor Author

Piinks commented Feb 23, 2022

Caught up with @HansMuller offline, the customizations afforded by MaterialButton and RawMaterialButton are achievable in the new button universe, and so we should deprecate the classes that are currently described as obsolete. This will mean we need to migrate and update more aspects of the framework, but to provide clarity for users we are going to start this deprecation period to direct folks to the right API to prevent further confusion.

@Piinks
Copy link
Contributor Author

Piinks commented Feb 23, 2022

I am going to close this in favor of a new PR

@Piinks Piinks closed this Feb 23, 2022
@rydmike
Copy link
Contributor

rydmike commented Feb 24, 2022

@Piinks & @HansMuller sounds good to me too.

I'm all for cleaning up and out these older APIs not in active use anymore. My goal while looking at them, was only to share what I found and shine a light on things that might need to be addressed. I know you have it well covered, but I guess an extra pair of eyes from someone that looks at and works with the theming classes quite a bit can't hurt, well other than being a pain in... 😅

There is a lot in ThemeData that needs this, and I see it is finally proceeding well, I've been tracking it quite closely and appreciate all the coming improvements.

While the transition might cause some difficulty, confusion and perhaps even a bit of frustration among Flutter devs during the transition. I can already see that the Flutter SDK's theming will become much clearer and consistent when it is completed. I think it will be worth some minor transition pain to get there. Of course as devs we always prefer non-breaking slow transitions, but sometimes a push and quick band-aid pull-off is needed.

I'll add more theming related findings if I come across any. I recently suggested two more colors to add to the ThemeData update collection issue. Not sure why they are not mentioned there already: #91772 (comment). It can hardly be intended for them to remain in ThemeData either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: annoyance Repeatedly frustrating issues with non-experimental functionality a: quality A truly polished experience d: api docs Issues with https://api.flutter.dev/ f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
3 participants