Skip to content

Conversation

PurplePolyhedron
Copy link
Contributor

When primary is false, Scaffold is not at the top of screen, so it should not have a status bar.
fixes #175062

Pre-launch Checklist

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 10, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly addresses the issue where a Scaffold with primary: false would incorrectly include a status bar gesture detector. The change is simple and effective, wrapping the status bar logic in a condition that checks widget.primary. The accompanying tests are well-written, covering both the intended behavior and preventing potential regressions. I have one minor suggestion to fix a typo in a test description.

removeRightPadding: false,
removeBottomPadding: true,
);
if (widget.primary) {
Copy link
Contributor

@navaronbracke navaronbracke Sep 10, 2025

Choose a reason for hiding this comment

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

Drive-by nit: This can use an early return instead, to reduce the diff / indent?

if (!widget.primary) {
  break;
}

_addIfNonNull(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Thanks for the suggestion

@dkwingsmt dkwingsmt self-requested a review September 17, 2025 18:06
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

switch (themeData.platform) {
case TargetPlatform.iOS:
case TargetPlatform.macOS:
if (!widget.primary) {
Copy link
Contributor

@dkwingsmt dkwingsmt Oct 1, 2025

Choose a reason for hiding this comment

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

Can you also add a paragraph explaining this to the document of [primary]?

@dkwingsmt dkwingsmt requested a review from Piinks October 1, 2025 18:30
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of doc nits, thanks for the contribution!

/// If true then the height of the [appBar] will be extended by the height
/// of the screen's status bar, i.e. the top padding for [MediaQuery].
///
/// If ture, on iOS and macOS, tapping the status bar scrolls the app's
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
/// If ture, on iOS and macOS, tapping the status bar scrolls the app's
/// If true, on iOS and macOS, tapping the status bar scrolls the app's


// On iOS, tapping the status bar scrolls the app's primary scrollable to the
// top. We implement this by looking up the primary scroll controller and
// On iOS and macOS, tapping the status bar scrolls the app's primary scrollable
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
// On iOS and macOS, tapping the status bar scrolls the app's primary scrollable
// On iOS and macOS, if `primary` is true, tapping the status bar scrolls the app's primary scrollable

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 8, 2025
Copy link
Contributor

auto-submit bot commented Oct 8, 2025

autosubmit label was removed for flutter/flutter/175156, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 8, 2025
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 8, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Oct 9, 2025
Merged via the queue into flutter:master with commit 07928d5 Oct 9, 2025
80 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

In second Scaffold in an Overlay, the AppBar does not response to gesture

4 participants