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

Implement CupertinoMenuAnchor and CupertinoMenuItem #143712

Draft
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

davidhicks980
Copy link
Contributor

@davidhicks980 davidhicks980 commented Feb 19, 2024

This PR implements CupertinoMenuAnchor, CupertinoMenuItem, CupertinoLargeMenuDivider using MenuAnchor as a base.

Resolves #60298, notDmDrl/pull_down_button#26, #137936

Tests modeled after MenuAnchor have been added to cover the API surface... perhaps too many tests. I split CupertinoMenuItem tests from CupertinoMenuAnchor tests to improve performance.

Four examples have been added with tests

  • Simple example demonstrating CupertinoMenuItem styling
  • Menu button example based on material/menu_anchor.0.dart
  • Context menu example based on simplified material/menu_anchor.1.dart
  • Nested example that replicates the iOS history stack. This one is more complex and may need to be removed.

Before implementing tests for the modifications made to MenuAnchor (a MenuAnchor.withOverlayBuilder constructor was added), I wanted to get some feedback. I initially made MenuAnchorState public and made the overlayBuilder a protected method, but found that adding a separate constructor required fewer changes and felt simpler overall. The tradeoff is that users would need to know how to properly set up semantics, focus, and actions if they were to use this constructor.

Also, I'm curious whether it would be valuable for me to fullfill this PR: #135025. I attempted to build a generic Panel widget that would encompass functionality and animations without providing structure, but I decided to stick to a wrapper until I heard feedback.

Some features that were going to be included in a future PR made it into this PR because they were either necessary to fulfill a core functionality of the menu (_PanRegion, which is modeled after _TapRegion, and _CupertinoMenuDivider), or they were so simple that it'd be more work to leave them out (CupertinoLargeMenuDivider). In the case of the former, all classes are private, fully documented, and have been tested in the context of their use in the menu anchor. With regard to CupertinoLargeMenuDivider, it has tests, documentation, and a public API.

A few loose ends:

  • LocalizedShortcutLabeler was made public rather than implementing a Cupertino version, seeing as it appears to have already accounted for iOS and MacOS and has public documentation/tests
  • Should accelerators be included?
  • Should the ability to customize the menu surface be removed?
  • To account for features like a sticky header, the menu's default scrollable is a CustomScrollView that is shrink wrapped. This introduces a performance penalty by default, although shrink wrapping can be disabled. Is there a better alternative?

Last, CupertinoMenuItem, CupertinoMenuAnchor, MenuItemButtons, and MenuAnchor all interop and can be swapped out pretty easily.

Screen.Recording.2024-02-19.at.12.47.24.PM.mov

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
    • Missing MenuAnchor tests
  • All existing and new tests are passing.

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

@Piinks @MitchellGoodwin Sorry in advance if you guys already saw this, but I forgot to tag you both

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Feb 19, 2024
@goderbauer
Copy link
Member

Hello @davidhicks980, sorry for the delay on this one. It looks like this change is failing some checks. Could you take a look at those and fix the issues? Thanks.

@davidhicks980
Copy link
Contributor Author

davidhicks980 commented Apr 24, 2024

@goderbauer Sure thing! I'll take a look later this week.

Edit 04/29: I have to make a few changes to accommodate text height differences across platforms (particularly browser), and only got around to some of the changes this weekend. I'll have more time later this week/weekend to finish up.

@davidhicks980
Copy link
Contributor Author

davidhicks980 commented May 10, 2024

Okay, assuming I correctly fixed the last four linux warnings, this should be good for a prelim look!

There is a lot of weirdness in this PR, particularly with regard to color blending. I tried to explain most of it, but please let me know if anything doesn't make sense.

Also, as I've already mentioned, the _PanRegion and related classes were added to match native behavior, but it may make sense to split this feature off into a separate file. I wanted to get feedback on it before doing so, as the implementation I've written is primitive (e.g. there is no way to isolate pans to a single region). I'd also be curious to hear whether we should incorporate menu text theming into the CupertinoTheme class... but I was afraid to add too much complexity with this commit.

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.

So excited about this! I have not reviewed all of the PR, the import of material into cupertino will need to be resolved first.

Also, in the earlier PR, we talked about splitting this up over multiple PRs. A change of this size is difficult to review and verify test coverage for. There are some changes in the material library in addition to new cupertino widgets here. Is it possible we can split this up a bit?

Comment on lines +17 to +19
import '../material/material_localizations.dart'
show MaterialLocalizations;
import '../material/menu_anchor.dart'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a valid import, the Cupertino library cannot import the material library, since material already imports cupertino. It creates a circular dependency between the libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The circular dependency problem is something I was unsure of how to fix. I asked a (long winded) question in the Discord chat, but no one got back to me (see here). Would you suggest I try moving the MenuAnchor core to the Widgets library, or do you have a suggestion? I'm mostly concerned about compromising any plans you all have, but I wasn't sure who to ask.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you suggest I try moving the MenuAnchor core to the Widgets library

This might be an option, let me ask some folks about it and circle back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I'll see what parts I can shed off of the CupertinoMenuAnchor to make it smaller.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving MenuAnchor and MenuController --> widgets SGT-everyone I spoke with! Is this something you are interested in doing in a separate change? If not, I can send a PR. :)

The only material-specific bit we would need to leave in material would be MenuAnchor.menuStyle. So this would probably look like a RawMenuAnchor class in widgets, with MenuAnchor as a subclass in material so it can have the MenuStyle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exciting! I'd be happy to. I believe I tried in the past and there are a few tricky dependencies between the two (mostly related to _Submenu, which depends on _MenuAnchorState internals, material theming, and DismissMenuAction, and MenuDirectionalFocusAction) but I'll go ahead and get a PR in so that we can discuss. I'll be out-of-town this weekend, but will submit ASAP!

const bool _kDebugMenus = false;

/// Whether [defaultTargetPlatform] is an Apple platform (Mac or iOS).
bool get _isApple {
Copy link
Contributor

Choose a reason for hiding this comment

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

We would use _isCupertino here. Avoiding brand names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! This was in the material repo so I submitted a PR here: #148432.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository 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.

Pull-Down Menus for iOS 14
3 participants