flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
162.2k stars 26.64k forks source link

Implement CupertinoMenuAnchor and CupertinoMenuItem #143712

Open davidhicks980 opened 2 months ago

davidhicks980 commented 2 months ago

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

Resolves https://github.com/flutter/flutter/issues/60298, https://github.com/notDmDrl/pull_down_button/issues/26, https://github.com/flutter/flutter/pull/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

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: https://github.com/flutter/flutter/issues/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:

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

https://github.com/flutter/flutter/assets/59215665/535295d3-e7dd-46ad-8682-a3cba79d2db9

Pre-launch Checklist

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

goderbauer commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 5 days ago

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.