-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[Framework] Add Share to selection controls #132599
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
[Framework] Add Share to selection controls #132599
Conversation
hellohuanlin
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 after addressing those comments
| String get searchWebButtonLabel => 'Search Web'; | ||
|
|
||
| @override | ||
| String get shareButtonLabel => 'Share...'; |
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.
can't remember - does native ios has the ...?
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.
| skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu. | ||
| ); | ||
|
|
||
| testWidgets('Share shows up on iOS only (CupertinoTextField)', (WidgetTester tester) async { |
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.
does share show up in material for ios?
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.
no (?), looks like cupertino to me
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.
Regardless of whether TextField or CupertinoTextField is being used, the default context menu will reflect the current platform. So if you use TextField on an iPhone, you'll still get the iOS context menu.
So this test and the following test look correct as written to me. The only thing I would change is to move the TextField test below to material/text_field_test.dart.
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 with nits 👍
| } | ||
|
|
||
| /// Launch the share interface for the current selection, | ||
| /// as in the "Share" edit menu button on iOS. |
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: Some weird indentation here after the ///.
| /// Launch the share interface for the current selection, | ||
| /// as in the "Share" edit menu button on iOS. | ||
| /// | ||
| /// Currently this is only implemented for iOS. |
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: Add an empty line of /// after this line.
| skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu. | ||
| ); | ||
|
|
||
| testWidgets('Share shows up on iOS only (CupertinoTextField)', (WidgetTester tester) async { |
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.
Regardless of whether TextField or CupertinoTextField is being used, the default context menu will reflect the current platform. So if you use TextField on an iPhone, you'll still get the iOS context menu.
So this test and the following test look correct as written to me. The only thing I would change is to move the TextField test below to material/text_field_test.dart.
| skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu. | ||
| ); | ||
|
|
||
| testWidgets('Share shows up on iOS only (TextField)', (WidgetTester tester) async { |
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.
As mentioned above, this test should be in material/text_field_test.dart.
| await gesture.up(); | ||
| await tester.pump(); | ||
| expect(find.byType(CupertinoButton), findsNWidgets(3)); | ||
| expect(find.byType(CupertinoButton), findsNWidgets(4)); |
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: Seeing you change these numbers all over the place in these PRs makes me think we should have a better way to test this... expect(contextMenuButtonsShown(ContextMenuButtons.all)); or something like that.
That's probably beyond the scope of this PR, though. Also it might not be worth it if we don't add many more buttons. Just throwing out a thought.
… into material/text_field_test
|
auto label is removed for flutter/flutter/132599, due to - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |

In native iOS, users are able to select text and initiate a share menu, which provides several standard services, such as copy, sharing to social media, direct ability to send to various contacts through messaging apps, etc.
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-09.at.18.47.02.mp4
This PR is the framework portion of the changes that will allow Share to be implemented.
The corresponding merged engine PR is here
This PR addresses #107578
More details are available in this design doc
Pre-launch Checklist
///).