-
Notifications
You must be signed in to change notification settings - Fork 29.5k
Correct position for an open CupertinoContextMenu #170943
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
Correct position for an open CupertinoContextMenu #170943
Conversation
|
good improvements, here are some other notes:
|
These sound like valid changes. To make it easier to review, this PR can focus on positioning/layout, and we can focus on the animation and visual fidelity in a subsequent PR. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Can you make sure the points made in #170943 (comment) get captured as an issue before we forget?
Besides those small differences it looks pretty good to me. The layout seems identical now by my eye. Thanks for polishing this up.
| void _onTapCancel() { | ||
| _openController.removeListener(_listenerCallback); | ||
| if (_openController.isAnimating && _openController.value < _midpoint) { | ||
| _openController.reverse(); | ||
| } | ||
| _onTap(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I have a slight worry that someone will modify _onTap without realizing that it's also the handler for tap cancel and tap up. I don't have a good solution to that though, and I think it's fine as-is. Naming is hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... prefer revert or leave as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be really verbose and add a _onTapCompleted method, and have _onTap and the other methods call it instead. I don't have a strong opinion though, the code is all close together.
| } | ||
| } | ||
|
|
||
| class _ContextMenuAlignedChildrenDelegate extends MultiChildLayoutDelegate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of MultiChildLayoutDelegate, CupertinoContextMenu probably should have used it from the start.
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
## Larger-sized child ### Flutter https://github.com/user-attachments/assets/e120bdda-4236-480d-a85f-183b176bc2a9 ### Native https://github.com/user-attachments/assets/7577b0a0-9111-47e0-ad5d-f533a14310ea ## Smaller-sized child ### Flutter https://github.com/user-attachments/assets/98ba9ffe-715f-4ec6-8d33-e10a8ae69d7d ### Native https://github.com/user-attachments/assets/b6dbc891-5e01-4ac5-a362-d714328e46d3 ## Landscape mode ### Flutter https://github.com/user-attachments/assets/9191b8ba-e80a-4d88-b6c4-75876704c2aa ### Native https://github.com/user-attachments/assets/bb7dfe21-a87b-4eab-a2d2-fab35a7a4c70 <details> <summary>Flutter sample code</summary> ```dart import 'package:flutter/cupertino.dart'; void main() => runApp(const ContextMenuApp()); class ContextMenuApp extends StatelessWidget { const ContextMenuApp({super.key}); @OverRide Widget build(BuildContext context) { return CupertinoApp( home: MyStatelessWidget(), ); } } /// This is the stateless widget that the main application instantiates. class MyStatelessWidget extends StatelessWidget { const MyStatelessWidget({super.key}); List<String> getDataList() { List<String> list = []; for (int i = 0; i < 15; i++) { list.add(i.toString()); } return list; } List<Widget> getWidgetList(BuildContext context) { return getDataList() .map((item) => getItemContainer(context, item)) .toList(); } Widget getItemContainer(BuildContext context, String item) { return CupertinoContextMenu( actions: List<Widget>.generate( 4, (index) { return CupertinoContextMenuAction( child: Text('Action ${index + 1}'), onPressed: () { Navigator.pop(context); }, ); }, ), child: SizedBox.square( dimension: 350, // Change to increase the size of the context menu's child. child: ColoredBox( color: CupertinoColors.destructiveRed, child: Align( alignment: Alignment.center, child: Text( item, style: TextStyle(color: CupertinoColors.white, fontSize: 20), ), ), ), ), ); } @OverRide Widget build(BuildContext context) { return CupertinoPageScaffold( navigationBar: CupertinoNavigationBar( middle: Text('Context Menu Demo'), ), child: SafeArea( child: GridView.count( crossAxisSpacing: 5.0, mainAxisSpacing: 5.0, padding: EdgeInsets.all(10.0), crossAxisCount: 3, childAspectRatio: 1.0, children: getWidgetList(context), ), ), ); } } ``` </details> <details> <summary>Native sample code</summary> ```swift import SwiftUI struct ContentView: View { let colors: [Color] = [.red, .orange, .blue, .green] var body: some View { NavigationView { ScrollView { LazyVGrid(columns: Array(repeating: GridItem(.flexible()), count: 3), spacing: 5) { ForEach(0..<15) { index in ContextMenuItem(item: "\(index)", color: colors.randomElement()!) } } .padding(10) } .navigationTitle("Context Menu Demo") .navigationBarTitleDisplayMode(.inline) } } } struct ContextMenuItem: View { let item: String let color: Color var body: some View { Text(item) .font(.system(size: 20)) .foregroundColor(.white) .frame(width: 220, height: 220) .background(color) .cornerRadius(8) .contextMenu { ForEach(1..<5) { i in Button("Action \(i)") { print("Action \(i) tapped for item: \(item)") } } } preview: { Text(item) .font(.system(size: 20)) .foregroundColor(.white) .frame(width: 250, height: 500) .background(color) .cornerRadius(8) } } } ``` </details> Fixes [iOS Context Menus incorrectly moves to the center between screens](flutter#60334)
…75300) Missed this in [Correct position for an open CupertinoContextMenu](flutter#170943) Fixes [CupertinoContextMenu wrong position](flutter#175225)
Larger-sized child
Flutter
bigger.children.mov
Native
portrait.native.large.child.mov
Smaller-sized child
Flutter
smaller.children.mov
Native
portrait.native.small.child.mov
Landscape mode
Flutter
context.menu.landscape.mov
Native
landscape.native.mov
Flutter sample code
Native sample code
Fixes iOS Context Menus incorrectly moves to the center between screens