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

Add clipBehavior to DialogTheme #147635

Conversation

ValentinVignal
Copy link
Contributor

Fixes #147634

Pre-launch Checklist

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

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 1, 2024
Comment on lines 1608 to +1610
elevation: 24.0,
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0))),
clipBehavior: Clip.none,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified this one because the comment says it is hand-coded. I didn't modify the material 3 ones below because the comments say it is being generated.

@TahaTesser, can I ask what I should do ?

Should I

  1. Not modify those default classes and leave the ?? Clip.none above in the code
  2. Modify on M2 by hand (current state of this PR)
  3. Modify M2 by and but also M3 (in which case, how to modify M3?)

?

Copy link
Member

Choose a reason for hiding this comment

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

You can modify _DialogDefaultsM2 as well as _DialogDefaultsM3.

To update M3 defaults go to flutter/dev/tools/gen_defaults/lib/dialog_template.dart file and update the file as like this:

class _${blockName}DefaultsM3 extends DialogTheme {
  _${blockName}DefaultsM3(this.context)
    : super(
        alignment: Alignment.center,
        elevation: ${elevation("md.comp.dialog.container")},
        shape: ${shape("md.comp.dialog.container")},
+       clipBehavior: Clip.none,
      );
class DialogFullscreenTemplate extends TokenTemplate {
  const DialogFullscreenTemplate(super.blockName, super.fileName, super.tokens);

  @override
  String generate() => '''
class _${blockName}DefaultsM3 extends DialogTheme {
+  const _${blockName}DefaultsM3(this.context) : super(clipBehavior: Clip.none);

And generate the template following instructions in here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thank you for the help!

@ValentinVignal ValentinVignal force-pushed the flutter/theme/Add-clip-behavior-to-dialog-theme branch from 61d1c2e to 526235b Compare May 1, 2024 10:36
@ValentinVignal
Copy link
Contributor Author

I'm not sure why the tests for the documentation fail, is there something I didn't do correctly?

@TahaTesser TahaTesser force-pushed the flutter/theme/Add-clip-behavior-to-dialog-theme branch from 85397fb to 28d582f Compare May 6, 2024 07:47
Copy link
Member

@TahaTesser TahaTesser 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 the contribution! Looks like there are some minor issues to address.

Comment on lines 1608 to +1610
elevation: 24.0,
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0))),
clipBehavior: Clip.none,
Copy link
Member

Choose a reason for hiding this comment

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

You can modify _DialogDefaultsM2 as well as _DialogDefaultsM3.

To update M3 defaults go to flutter/dev/tools/gen_defaults/lib/dialog_template.dart file and update the file as like this:

class _${blockName}DefaultsM3 extends DialogTheme {
  _${blockName}DefaultsM3(this.context)
    : super(
        alignment: Alignment.center,
        elevation: ${elevation("md.comp.dialog.container")},
        shape: ${shape("md.comp.dialog.container")},
+       clipBehavior: Clip.none,
      );
class DialogFullscreenTemplate extends TokenTemplate {
  const DialogFullscreenTemplate(super.blockName, super.fileName, super.tokens);

  @override
  String generate() => '''
class _${blockName}DefaultsM3 extends DialogTheme {
+  const _${blockName}DefaultsM3(this.context) : super(clipBehavior: Clip.none);

And generate the template following instructions in here

packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/dialog_theme_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/dialog_theme_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/dialog_theme_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/dialog_theme_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/dialog_theme_test.dart Outdated Show resolved Hide resolved
@ValentinVignal
Copy link
Contributor Author

Thank you @TahaTesser for this very detailed review! Hopefully I fixed all the issues you found

@TahaTesser TahaTesser force-pushed the flutter/theme/Add-clip-behavior-to-dialog-theme branch from 2e6f05a to 9c51764 Compare May 7, 2024 15:51
TahaTesser

This comment was marked as resolved.

@TahaTesser TahaTesser requested a review from bleroux May 9, 2024 07:49
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

Nice! One small typo to fix before approval.

@@ -186,9 +186,10 @@ class Dialog extends StatelessWidget {
/// See the enum [Clip] for details of all possible options and their common
/// use cases.
///
/// Defaults to [Clip.none].
/// If null, then [DialogTheme.clipBehavior] is used. If that is also null,
/// the defaults to [Clip.none].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// the defaults to [Clip.none].
/// defaults to [Clip.none].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I committed it in Update packages/flutter/lib/src/material/dialog.dart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no! now the pipeline is failing because of your CLA @bleroux

Copy link
Contributor

@bleroux bleroux May 9, 2024

Choose a reason for hiding this comment

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

Oh no! now the pipeline is failing because of your CLA @bleroux

@ValentinVignal Oh, sorry for that, I just noticed that the GitHub web operations use the primary email address set in my GitHub account which is not the one I use for filing PR. I have updated it but this won't change this last commit on your PR.
So the solution I see is that you delete this last commit, create a similar one and force push. Really sorry for that 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sure, I'll do that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be good to go now 👍

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM!

@ValentinVignal ValentinVignal force-pushed the flutter/theme/Add-clip-behavior-to-dialog-theme branch from 4af8700 to e6e88e9 Compare May 10, 2024 01:14
@bleroux bleroux force-pushed the flutter/theme/Add-clip-behavior-to-dialog-theme branch from e6e88e9 to 58e8dc7 Compare May 10, 2024 06:38
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2024
@auto-submit auto-submit bot merged commit 8e53ad9 into flutter:master May 10, 2024
138 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 11, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 11, 2024
flutter/flutter@2bfb1b0...2aa05c1

2024-05-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from fad88cb16d03 to 558a81dd8b08 (3 revisions) (flutter/flutter#148163)
2024-05-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from ba8e0d3e2f23 to fad88cb16d03 (9 revisions) (flutter/flutter#148156)
2024-05-11 32538273+ValentinVignal@users.noreply.github.com Add test for scaffold.1.dart (flutter/flutter#147966)
2024-05-10 tessertaha@gmail.com Fix `MaterialStateBorderSide` lerp in the `Checkbox` and chips (flutter/flutter#148124)
2024-05-10 jmccandless@google.com Docs on TextField disposed by a scrollable (flutter/flutter#148149)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from d4f705ccb695 to ba8e0d3e2f23 (8 revisions) (flutter/flutter#148147)
2024-05-10 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#148148)
2024-05-10 32538273+ValentinVignal@users.noreply.github.com Add `clipBehavior` to `DialogTheme` (flutter/flutter#147635)
2024-05-10 31859944+LongCatIsLooong@users.noreply.github.com bump cupertino_icons to 1.08 (flutter/flutter#146806)
2024-05-10 sokolovskyi.konstantin@gmail.com Add test for animated_size.0.dart API example. (flutter/flutter#147828)
2024-05-10 120297255+PurplePolyhedron@users.noreply.github.com Fix `DropdownMenu` keyboard navigation (flutter/flutter#147294)
2024-05-10 sokolovskyi.konstantin@gmail.com Add test for draggable.0.dart API example. (flutter/flutter#147941)
2024-05-10 magder@google.com Update TESTOWNERS (flutter/flutter#148108)
2024-05-10 sokolovskyi.konstantin@gmail.com Add tests for stream_builder.0.dart API example. (flutter/flutter#147832)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1ccd0c308b3a to d4f705ccb695 (2 revisions) (flutter/flutter#148142)
2024-05-10 engine-flutter-autoroll@skia.org Roll Packages from 8de142d to 6c4482a (8 revisions) (flutter/flutter#148079)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0917b14fc36 to 1ccd0c308b3a (10 revisions) (flutter/flutter#148137)
2024-05-10 nate.w5687@gmail.com `if` chains � `switch` expressions (flutter/flutter#147793)
2024-05-10 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.1 to 2.3.3 (flutter/flutter#148091)
2024-05-10 31859944+LongCatIsLooong@users.noreply.github.com Reland "Implement computeDryBaseline for `RenderWrap` (#146260)" (flutter/flutter#148086)
2024-05-10 magder@google.com Update dependabot reviewers (flutter/flutter#148101)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6e722ae213bd to c0917b14fc36 (1 revision) (flutter/flutter#148084)
2024-05-09 christopherfujino@gmail.com Don't pin package:macros (flutter/flutter#148087)
2024-05-09 ian@hixie.ch Remove hidden dependencies on the default LocalPlatform (flutter/flutter#147342)
2024-05-09 nate.w5687@gmail.com Getting rid of containers (flutter/flutter#147432)
2024-05-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0fd3386d018 to 6e722ae213bd (2 revisions) (flutter/flutter#148070)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
…r#6713)

flutter/flutter@2bfb1b0...2aa05c1

2024-05-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from fad88cb16d03 to 558a81dd8b08 (3 revisions) (flutter/flutter#148163)
2024-05-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from ba8e0d3e2f23 to fad88cb16d03 (9 revisions) (flutter/flutter#148156)
2024-05-11 32538273+ValentinVignal@users.noreply.github.com Add test for scaffold.1.dart (flutter/flutter#147966)
2024-05-10 tessertaha@gmail.com Fix `MaterialStateBorderSide` lerp in the `Checkbox` and chips (flutter/flutter#148124)
2024-05-10 jmccandless@google.com Docs on TextField disposed by a scrollable (flutter/flutter#148149)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from d4f705ccb695 to ba8e0d3e2f23 (8 revisions) (flutter/flutter#148147)
2024-05-10 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#148148)
2024-05-10 32538273+ValentinVignal@users.noreply.github.com Add `clipBehavior` to `DialogTheme` (flutter/flutter#147635)
2024-05-10 31859944+LongCatIsLooong@users.noreply.github.com bump cupertino_icons to 1.08 (flutter/flutter#146806)
2024-05-10 sokolovskyi.konstantin@gmail.com Add test for animated_size.0.dart API example. (flutter/flutter#147828)
2024-05-10 120297255+PurplePolyhedron@users.noreply.github.com Fix `DropdownMenu` keyboard navigation (flutter/flutter#147294)
2024-05-10 sokolovskyi.konstantin@gmail.com Add test for draggable.0.dart API example. (flutter/flutter#147941)
2024-05-10 magder@google.com Update TESTOWNERS (flutter/flutter#148108)
2024-05-10 sokolovskyi.konstantin@gmail.com Add tests for stream_builder.0.dart API example. (flutter/flutter#147832)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1ccd0c308b3a to d4f705ccb695 (2 revisions) (flutter/flutter#148142)
2024-05-10 engine-flutter-autoroll@skia.org Roll Packages from 8de142d to 6c4482a (8 revisions) (flutter/flutter#148079)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0917b14fc36 to 1ccd0c308b3a (10 revisions) (flutter/flutter#148137)
2024-05-10 nate.w5687@gmail.com `if` chains � `switch` expressions (flutter/flutter#147793)
2024-05-10 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.1 to 2.3.3 (flutter/flutter#148091)
2024-05-10 31859944+LongCatIsLooong@users.noreply.github.com Reland "Implement computeDryBaseline for `RenderWrap` (#146260)" (flutter/flutter#148086)
2024-05-10 magder@google.com Update dependabot reviewers (flutter/flutter#148101)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6e722ae213bd to c0917b14fc36 (1 revision) (flutter/flutter#148084)
2024-05-09 christopherfujino@gmail.com Don't pin package:macros (flutter/flutter#148087)
2024-05-09 ian@hixie.ch Remove hidden dependencies on the default LocalPlatform (flutter/flutter#147342)
2024-05-09 nate.w5687@gmail.com Getting rid of containers (flutter/flutter#147432)
2024-05-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0fd3386d018 to 6e722ae213bd (2 revisions) (flutter/flutter#148070)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add clipBehavior to DialogTheme
3 participants