Skip to content

Let CupertinoPageScaffold have tap status bar to scroll to top#29946

Merged
xster merged 2 commits intoflutter:masterfrom
xster:cupertino-tap-to-top
Mar 27, 2019
Merged

Let CupertinoPageScaffold have tap status bar to scroll to top#29946
xster merged 2 commits intoflutter:masterfrom
xster:cupertino-tap-to-top

Conversation

@xster
Copy link
Member

@xster xster commented Mar 26, 2019 •
edited
Loading

Description

Let CupertinoPageScaffold have a primary scroll controller and a status bar tap handler.

Related Issues

#24777

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes tests for all changed/updated/fixed behaviors (See Test Coverage).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

@xster xster requested a review from matthew-carroll March 26, 2019 00:37
@Piinks Piinks added the f: cupertino flutter/packages/flutter/cupertino repository label Mar 26, 2019
matthew-carroll
matthew-carroll reviewed Mar 26, 2019
View reviewed changes
packages/flutter/lib/src/cupertino/page_scaffold.dart Outdated
Copy link
Contributor

@matthew-carroll matthew-carroll Mar 26, 2019

Choose a reason for hiding this comment

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

Can you add a comment that mentions why the hasClients condition is relevant to handling the tap?

Copy link
Member Author

@xster xster Mar 27, 2019

Choose a reason for hiding this comment

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

SG, added comment

packages/flutter/lib/src/cupertino/page_scaffold.dart Outdated
Copy link
Contributor

@matthew-carroll matthew-carroll Mar 26, 2019

Choose a reason for hiding this comment

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

This doesn't appear to be padded content?

Copy link
Member Author

@xster xster Mar 27, 2019

Choose a reason for hiding this comment

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

Moved this addition to the end

packages/flutter/lib/src/cupertino/page_scaffold.dart Outdated
Copy link
Contributor

@matthew-carroll matthew-carroll Mar 26, 2019

Choose a reason for hiding this comment

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

Is this an extra blank line?

Copy link
Member Author

@xster xster Mar 27, 2019

Choose a reason for hiding this comment

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

Thanks, it is. Removed.

packages/flutter/lib/src/cupertino/page_scaffold.dart Outdated
Copy link
Contributor

@matthew-carroll matthew-carroll Mar 26, 2019

Choose a reason for hiding this comment

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

Can you add a comment about what this stacked addition is doing? It can be difficult to understand the steps in assembling a layout in these build functions.

Copy link
Member Author

@xster xster Mar 27, 2019

Choose a reason for hiding this comment

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

Added comment. Thanks

packages/flutter/test/cupertino/scaffold_test.dart Outdated
Copy link
Contributor

@matthew-carroll matthew-carroll Mar 26, 2019

Choose a reason for hiding this comment

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

Is this done to imitate a status bar? If so, it would be great if we could codify that as something more than what looks like incidental padding. Maybe a status bar widget just for testing?

Copy link
Member Author

@xster xster Mar 27, 2019

Choose a reason for hiding this comment

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

Added comment for now to avoid an indirection. Makes sense to make a function or something if there were any more status bar tests in the future.

@xster xster force-pushed the cupertino-tap-to-top branch from 391e639 to 92b3789 Compare March 27, 2019 00:38
@xster xster merged commit 7f3485e into flutter:master Mar 27, 2019
@xster xster deleted the cupertino-tap-to-top branch March 27, 2019 20:52
@gaaclarke gaaclarke mentioned this pull request Sep 26, 2019
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@matthew-carroll matthew-carroll matthew-carroll approved these changes

Assignees

No one assigned

Labels

f: cupertino flutter/packages/flutter/cupertino repository

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xster @matthew-carroll @googlebot @Piinks