Uh oh!
There was an error while loading. Please reload this page.
[flutter_adaptive_scaffold] Add expanded and extra large breakpoints - #7300
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
fb15c81 to
e56c89aComparemartijn00
commented
Aug 7, 2024
@gspencergoog I'm looking a bit at refactoring the breakpoints into an enhanced enum. enumBreakpoints {
standard(begin:-1),
small(begin:0, end:600, bottom:480),
medium(begin:600, end:840, top:480, bottom:900),
mediumLarge(begin:840, end:1200, top:900),
large(begin:1200, end:1600, top:900),
extraLarge(begin:1600, top:900);
constBreakpoints({
requiredthis.begin,
this.end,
this.top,
this.bottom,
this.platform,
});
finaldouble begin;
finaldouble? end;
finaldouble? top;
finaldouble? bottom;
finalSet<TargetPlatform>? platform;
boolisActive(BuildContext context, {bool up =false, bool isDesktop =false, bool isMobile =false}) {
finalTargetPlatform host =Theme.of(context).platform;
finalbool isRightPlatform = platform?.contains(host) ??true;
finaldouble width =MediaQuery.sizeOf(context).width;
finaldouble height =MediaQuery.sizeOf(context).height;
finalOrientation orientation =MediaQuery.orientationOf(context);
finaldouble lowerBoundWidth = begin;
finaldouble upperBoundWidth = end ??double.infinity;
finaldouble lowerBoundHeight = top ??double.negativeInfinity;
finaldouble upperBoundHeight = bottom ??double.infinity;
finalbool isWidthActive = up ? width >= lowerBoundWidth : width >= lowerBoundWidth && width < upperBoundWidth;
finalbool isHeightActive = (orientation ==Orientation.landscape &&
height >= lowerBoundHeight &&
height < upperBoundHeight) ||
orientation ==Orientation.portrait;
bool isPlatformActive =true;
if (isDesktop) {
isPlatformActive = _desktop.contains(host);
} elseif (isMobile) {
isPlatformActive = _mobile.contains(host);
}
return isWidthActive && isHeightActive && isRightPlatform && isPlatformActive;
}
}This would enable users to better check on the enum. What do you think about that? |
gspencergoog
commented
Aug 7, 2024
Ooh, I like that better. You'd be renaming the |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Since we're still pre-1.0 here, no need to bump to 1.0, but since you're making breaking changes, you might want to bump to 0.2.0 and note the breaking changes in the CHANGELOG.
There was a problem hiding this comment.
If that's the case, it would be great to make some more breaking changes hehe. I have some more ideas for bigger changes to align with Material specs. I would like to land those in smaller pieces in new PRs otherwise it would all just get one massive PR.
There was a problem hiding this comment.
Sure, we can take a look at those, but let's not put them in this PR. Best to keep the breaking changes in separate PRs so they can be more easily reverted if needed.
I didn't mean to suggest a field day for breaking changes. Let's still consider them on their merits before breaking people, even if it's a pre-1.0 package. But if they're worth it, by all means!
martijn00
commented
Aug 7, 2024
@gspencergoog i've got this now. enumBreakpoint {
/// This is a standard breakpoint that can be used as a fallthrough in the /// case that no other breakpoint is active. /// /// It is active from a width of -1 dp to infinity.standard(beginWidth:-1),
/// A window whose width is less than 600 dp, greater than 0 dp and /// whose height is less than 480 dp.small(beginWidth:0, endWidth:600, endHeight:480),
/// A window whose width is less than 840 dp, greater than 600 dp and /// whose height is less than 900 dp.medium(beginWidth:600, endWidth:840, beginHeight:480, endHeight:900),
/// A window whose width is less than 1200 dp, greater than 840 dp and /// whose height is more than 900 dp.mediumLarge(beginWidth:840, endWidth:1200, beginHeight:900),
/// A window whose width is less than 1600 dp, greater than 1200 dp and /// whose height is more than 900 dp.large(beginWidth:1200, endWidth:1600, beginHeight:900),
/// A window whose width is greater than 1600 dp and whose height is more /// than 900 dp.extraLarge(beginWidth:1600, beginHeight:900);
constBreakpoint({
this.beginWidth,
this.endWidth,
this.beginHeight,
this.endHeight,
});
/// The beginning width dp value. If left null then the [Breakpoint] will have /// no lower bound.finaldouble? beginWidth;
/// The end width dp value. If left null then the [Breakpoint] will have no /// upper bound.finaldouble? endWidth;
/// The beginning height dp value. If left null then the [Breakpoint] will have /// no lower bound.finaldouble? beginHeight;
/// The end height dp value. If left null then the [Breakpoint] will have no /// upper bound.finaldouble? endHeight;
/// Returns true if the [Breakpoint] is active based on the conditions of the /// [BuildContext].boolisActive(
BuildContext context, {
bool andUp =false,
Set<TargetPlatform>? targetPlatform,
}) {
finalTargetPlatform host =Theme.of(context).platform;
finalbool isRightPlatform = targetPlatform?.contains(host) ??true;
finaldouble width =MediaQuery.sizeOf(context).width;
finaldouble height =MediaQuery.sizeOf(context).height;
finalOrientation orientation =MediaQuery.orientationOf(context);
finaldouble lowerBoundWidth = beginWidth ??double.negativeInfinity;
finaldouble upperBoundWidth = endWidth ??double.infinity;
finaldouble lowerBoundHeight = beginHeight ??double.negativeInfinity;
finaldouble upperBoundHeight = endHeight ??double.infinity;
finalbool isWidthActive = andUp
? width >= lowerBoundWidth
: width >= lowerBoundWidth && width < upperBoundWidth;
finalbool isHeightActive = (orientation ==Orientation.landscape &&
height >= lowerBoundHeight &&
height < upperBoundHeight) ||
orientation ==Orientation.portrait;
return isWidthActive && isHeightActive && isRightPlatform;
}
}The problem with this approach is that you can't extend it. You wouldn't be able to do something like: Do you have any ideas on how to do this? |
diegotori
commented
Aug 7, 2024
Enhanced enums aren't suitable for this use case, since it would prevent a dev such as myself from defining custom breakpoints outside of the Material 3 spec. In other words, before your PR, I had to create custom If you go through with this change, if and when the Material Design team decides to update the breakpoints again in the future and they don't end up making it into this library immediately, then there will be no way to get around that limitation. |
gspencergoog
commented
Aug 8, 2024
Ahh, you're right, that's not going to work. Sorry, I should have realized that when you suggested it. |
martijn00
commented
Aug 8, 2024
@gspencergoog I've tried a bit more and come up with this: classBreakpoint {
/// Returns a const [Breakpoint] with the given constraints.constBreakpoint({
this.beginWidth,
this.endWidth,
this.beginHeight,
this.endHeight,
this.platform,
this.andUp =false,
});
constBreakpoint.small({this.andUp =false, this.platform})
: beginWidth =0,
endWidth =600,
beginHeight =null,
endHeight =480;
constBreakpoint.medium({this.andUp =false, this.platform})
: beginWidth =600,
endWidth =840,
beginHeight =480,
endHeight =900;
staticconstSet<TargetPlatform> desktop =<TargetPlatform>{
TargetPlatform.linux,
TargetPlatform.macOS,
TargetPlatform.windows
};
staticconstSet<TargetPlatform> mobile =<TargetPlatform>{
TargetPlatform.android,
TargetPlatform.fuchsia,
TargetPlatform.iOS,
};
/// When set to true, it will include any size above the set width.finalbool andUp;
/// The beginning width dp value. If left null then the [Breakpoint] will have /// no lower bound.finaldouble? beginWidth;
/// The end width dp value. If left null then the [Breakpoint] will have no /// upper bound.finaldouble? endWidth;
/// The beginning height dp value. If left null then the [Breakpoint] will have /// no lower bound.finaldouble? beginHeight;
/// The end height dp value. If left null then the [Breakpoint] will have no /// upper bound.finaldouble? endHeight;
/// A Set of [TargetPlatform]s that the [Breakpoint] will be active on. If /// left null then it will be active on all platforms.finalSet<TargetPlatform>? platform;
/// A method that returns true based on conditions related to the context of /// the screen such as MediaQuery.sizeOf(context).width.boolisActive(BuildContext context) {
finalTargetPlatform host =Theme.of(context).platform;
finalbool isRightPlatform = platform?.contains(host) ??true;
finaldouble width =MediaQuery.sizeOf(context).width;
finaldouble height =MediaQuery.sizeOf(context).height;
finalOrientation orientation =MediaQuery.orientationOf(context);
finaldouble lowerBoundWidth = beginWidth ??double.negativeInfinity;
finaldouble upperBoundWidth = endWidth ??double.infinity;
finaldouble lowerBoundHeight = beginHeight ??double.negativeInfinity;
finaldouble upperBoundHeight = endHeight ??double.infinity;
finalbool isWidthActive = andUp
? width >= lowerBoundWidth
: width >= lowerBoundWidth && width < upperBoundWidth;
finalbool isHeightActive = (orientation ==Orientation.landscape &&
height >= lowerBoundHeight &&
height < upperBoundHeight) ||
orientation ==Orientation.portrait;
return isWidthActive && isHeightActive && isRightPlatform;
}
}Now you can easily use it like this: I can forward the existing code like this: This would remove the What do you think? |
gspencergoog
commented
Aug 8, 2024
Yes, that looks better, and still extensible. |
Uh oh!
There was an error while loading. Please reload this page.
@gspencergoog If you merge #7310 first i'll rebase this one and update the changelog again before the release to pub. |
martijn00
commented
Aug 8, 2024
@gspencergoog rebase is done. |
martijn00
commented
Aug 9, 2024
@Renzo-Olivares@gspencergoog fixed the comments. |
Uh oh!
There was an error while loading. Please reload this page.
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM w/ last small nit. Thank you for the contribution!
martijn00
commented
Aug 10, 2024
flutter/packages@f7b1256...d9a6de8 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/camera/camera_android/android (flutter/packages#7371) 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/path_provider/path_provider_android/android (flutter/packages#7376) 2024-08-12 tarrinneal@gmail.com [pigeon] removes restriction on number of custom types per file (flutter/packages#6840) 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.0 to 2.0.10 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7370) 2024-08-12 fertrig@gmail.com [shared_preferences] Fixes get-all when suite name is used (flutter/packages#7335) 2024-08-12 mhvdijk@gmail.com [flutter_adaptive_scaffold] Add expanded and extra large breakpoints (flutter/packages#7300) 2024-08-12 engine-flutter-autoroll@skia.org Manual roll Flutter from b12d861 to 9b84701 (8 revisions) (flutter/packages#7366) 2024-08-10 engine-flutter-autoroll@skia.org Manual roll Flutter from 76107bd to b12d861 (14 revisions) (flutter/packages#7358) 2024-08-09 tarrinneal@gmail.com [shared_preferences] fix cast error and mutable list error with `getStringList` (flutter/packages#7355) 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-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
| ? width >= lowerBoundWidth | ||
| : width >= lowerBoundWidth && width < upperBoundWidth; | ||
| final bool isHeightActive = (orientation == Orientation.landscape && |
There was a problem hiding this comment.
This breaks desktops. Height is irrelevant for desktops, regardless of orientation, imo :(
@martijn00
flutter/packages@f7b1256...d9a6de8 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/camera/camera_android/android (flutter/packages#7371) 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/path_provider/path_provider_android/android (flutter/packages#7376) 2024-08-12 tarrinneal@gmail.com [pigeon] removes restriction on number of custom types per file (flutter/packages#6840) 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.0 to 2.0.10 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7370) 2024-08-12 fertrig@gmail.com [shared_preferences] Fixes get-all when suite name is used (flutter/packages#7335) 2024-08-12 mhvdijk@gmail.com [flutter_adaptive_scaffold] Add expanded and extra large breakpoints (flutter/packages#7300) 2024-08-12 engine-flutter-autoroll@skia.org Manual roll Flutter from b12d861 to 9b84701 (8 revisions) (flutter/packages#7366) 2024-08-10 engine-flutter-autoroll@skia.org Manual roll Flutter from 76107bd to b12d861 (14 revisions) (flutter/packages#7358) 2024-08-09 tarrinneal@gmail.com [shared_preferences] fix cast error and mutable list error with `getStringList` (flutter/packages#7355) 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-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
flutter/packages@f7b1256...d9a6de8 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/camera/camera_android/android (flutter/packages#7371) 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/path_provider/path_provider_android/android (flutter/packages#7376) 2024-08-12 tarrinneal@gmail.com [pigeon] removes restriction on number of custom types per file (flutter/packages#6840) 2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.0 to 2.0.10 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7370) 2024-08-12 fertrig@gmail.com [shared_preferences] Fixes get-all when suite name is used (flutter/packages#7335) 2024-08-12 mhvdijk@gmail.com [flutter_adaptive_scaffold] Add expanded and extra large breakpoints (flutter/packages#7300) 2024-08-12 engine-flutter-autoroll@skia.org Manual roll Flutter from b12d861 to 9b84701 (8 revisions) (flutter/packages#7366) 2024-08-10 engine-flutter-autoroll@skia.org Manual roll Flutter from 76107bd to b12d861 (14 revisions) (flutter/packages#7358) 2024-08-09 tarrinneal@gmail.com [shared_preferences] fix cast error and mutable list error with `getStringList` (flutter/packages#7355) 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-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
…lutter#7300) This aligns the material3 specs for window sizes with the official docs: https://m3.material.io/foundations/layout/applying-layout/expanded. This adds checks for height based breakpoints: https://developer.android.com/develop/ui/compose/layouts/adaptive/window-size-classes I would like to rename `smallBody` to `compactBody` to align that as well but that would be a breaking change. Let me know if you want that to happen. *List which issues are fixed by this PR. You must list at least one issue.* flutter/flutter#118932
…lutter#7300) This aligns the material3 specs for window sizes with the official docs: https://m3.material.io/foundations/layout/applying-layout/expanded. This adds checks for height based breakpoints: https://developer.android.com/develop/ui/compose/layouts/adaptive/window-size-classes I would like to rename `smallBody` to `compactBody` to align that as well but that would be a breaking change. Let me know if you want that to happen. *List which issues are fixed by this PR. You must list at least one issue.* flutter/flutter#118932

This aligns the material3 specs for window sizes with the official docs: https://m3.material.io/foundations/layout/applying-layout/expanded.
This adds checks for height based breakpoints: https://developer.android.com/develop/ui/compose/layouts/adaptive/window-size-classes
I would like to rename
smallBodytocompactBodyto align that as well but that would be a breaking change. Let me know if you want that to happen.List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#118932
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.