这是indexloc提供的服务，不要输入任何密码
Skip to content

Pass MaterialButton.disabledElevation into RawMaterialButton #58209

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
merged 8 commits into from
Jun 3, 2020
Merged

Pass MaterialButton.disabledElevation into RawMaterialButton #58209

merged 8 commits into from
Jun 3, 2020

Conversation

pedromassango
Copy link
Member

Description

This PR passes the disabledElevation parameter of MaterialButton into its super widget.

Related Issues

#58201

Tests

I added the following tests:

  • Make sure that disabledElevation parameter of MaterialButton widget is passed into RawMaterialButton widget wich is its super class.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 28, 2020
@pedromassango
Copy link
Member Author

@shihaohong can you review this? Thanks

HansMuller
HansMuller previously approved these changes May 29, 2020
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.

NICE. LGTM

@HansMuller
Copy link
Contributor

@pedromassango
The test failure looks legitimate, presumably a semantics tree expectation needs to be updated. Can you take a look at that?

@HansMuller HansMuller dismissed their stale review May 29, 2020 14:46

Test failures

@pedromassango
Copy link
Member Author

@pedromassango
The test failure looks legitimate, presumably a semantics tree expectation needs to be updated. Can you take a look at that?

Sure, I should fix this in the next couple of hours

@pedromassango
Copy link
Member Author

@pedromassango
The test failure looks legitimate, presumably a semantics tree expectation needs to be updated. Can you take a look at that?

Hi @HansMuller. This is happening because of this line:
assert(disabledElevation == null || disabledElevation >= 0.0),

The previous tests of MaterialButton widget was not taking this property into account. I'm planning to provide a top-level default value right in the MaterialButton widget. Is that right?

@HansMuller
Copy link
Contributor

Not sure I understand why we're tripping that assert? Is someone passing a negative value for disabledElevation to MaterialButton?

@pedromassango
Copy link
Member Author

Not sure I understand why we're tripping that assert? Is someone passing a negative value for disabledElevation to MaterialButton?

Actually no. the tests are not even using it. I will just set a default value: 0.0

@pedromassango
Copy link
Member Author

The comments of disabledElevation says that it defaults to 0.0 but this is not actually true, it defaults to null.

@pedromassango
Copy link
Member Author

@HansMuller any update on this?

shihaohong
shihaohong reviewed Jun 3, 2020
View reviewed changes
packages/flutter/lib/src/material/material_button.dart Outdated
@@ -68,7 +68,7 @@ class MaterialButton extends StatelessWidget {
this.focusElevation,
this.hoverElevation,
this.highlightElevation,
this.disabledElevation,
this.disabledElevation = 0.0,
Copy link
Contributor

@shihaohong shihaohong Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would modify the assert on line 90 to say

       assert(disabledElevation != null && disabledElevation >= 0.0),

Since RawMaterialButton does not allow a null disabledElevation anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds sensible to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. This would e a breaking change because the current constructor is const and doing this change requires to remove the const keyword!

Copy link
Contributor

Choose a reason for hiding this comment

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

Here some context for the problem I managed to find: dart-lang/sdk#36511

shihaohong
shihaohong reviewed Jun 3, 2020
View reviewed changes
shihaohong
shihaohong approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@shihaohong shihaohong left a comment

Choose a reason for hiding this comment

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

LGTM

@shihaohong shihaohong merged commit 997a6ff into flutter:master Jun 3, 2020
@shihaohong shihaohong mentioned this pull request Jun 3, 2020
Closed
13 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
@pedromassango pedromassango deleted the patch-4 branch January 22, 2022 10:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@HansMuller HansMuller HansMuller left review comments

@shihaohong shihaohong shihaohong approved these changes

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.
5 participants
@pedromassango @HansMuller @shihaohong @googlebot @fluttergithubbot