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

[quick_actions] add localizedSubtitle for iOS #6973

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GiacomoPignoni
Copy link

@GiacomoPignoni GiacomoPignoni commented Jun 23, 2024

Add the localizedSubtitle field on quick actions for iOS

#129759

Pre-launch Checklist

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

Copy link

google-cla bot commented Jun 23, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jmagman
Copy link
Member

jmagman commented Jun 24, 2024

I like this idea, though what you currently have doesn't compile because the quick_actions_platform_interface doesn't have that parameter. Check out https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-platform-interface-method-parameters

 lib/main.dart:56:9: Error: No named parameter with the name 'localizedSubtitle'.
         localizedSubtitle: 'Action one subtitle',
         ^^^^^^^^^^^^^^^^^
 ../../../../../.pub-cache/hosted/pub.dev/quick_actions_platform_interface-1.0.6/lib/types/shortcut_item.dart:11:9: Context: Found this candidate, but the arguments don't match.
   const ShortcutItem({

Also quick_actions_ios will need its CHANGELOG updated. Take a look at that wiki page for more details about how to contribute to packages.
Thank you!

@GiacomoPignoni
Copy link
Author

Ok sorry @jmagman, I've added the deps override (as indicated in the documentation), the quick_actions_ios CHANGELOG and also the quick_actions_platform_interface CHANGELOG.

Now I wait for the review, than I'll make a new PR to update only the platform interface

@jmagman
Copy link
Member

jmagman commented Jun 26, 2024

Swift format:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8744034268588324081/+/u/Run_package_tests/Swift_format/stdout

Are you able to reproduce this test timeout locally?

12:00 +0 -1: loading /Volumes/Work/s/w/ir/x/w/packages/packages/quick_actions/quick_actions/example/integration_test/quick_actions_test.dart [E]   

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8744034268487478385/+/u/Run_package_tests/drive_examples/stdout

@@ -1,5 +1,6 @@
## NEXT

* Add localizedSubtitle field for iOS
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the CHANGELOG style guide linked from the PR checklist.

@@ -26,3 +26,8 @@ dev_dependencies:

flutter:
uses-material-design: true

# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert all of the quick_actions_android override entries since the package won't need any changes.

@@ -1,3 +1,7 @@
## NEXT
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the versioning policy linked in from the PR checklist.

@@ -1,5 +1,6 @@
## NEXT
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, please see the versioning policy.

@@ -20,6 +21,11 @@ class ShortcutItem {
/// Localized title of the item.
final String localizedTitle;

/// Localized subtitle of the item.
///
/// This is ignored on Android, since it's not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Platform interface packages should not make claims about what implementation packages, which are a higher level, do or don't do. This should just say that it's ignored by platforms that don't support it.

@@ -34,7 +34,7 @@ Finally, manage the app's quick actions, for instance:
```dart
quickActions.setShortcutItems(<ShortcutItem>[
const ShortcutItem(type: 'action_main', localizedTitle: 'Main view', icon: 'icon_main'),
const ShortcutItem(type: 'action_help', localizedTitle: 'Help', icon: 'icon_help')
const ShortcutItem(type: 'action_help', localizedTitle: 'Help', localizedSubtitle: 'Tap to get help', icon: 'icon_help')
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me that this is a common enough option that we actually need to update the README and example app to demonstrate it.

@@ -92,6 +92,7 @@ public final class QuickActionsPlugin: NSObject, FlutterPlugin, IOSQuickActionsA

let type = shortcut.type
let localizedTitle = shortcut.localizedTitle
let localizedSubtitle = shortcut.localizedSubtitle
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be inlined below, as it doesn't seem like the local variables are adding any value here. (The ones above may well be left over from a direct Pigeon conversion, as they would have been more useful prior to Pigeon.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This change should have corresponding native testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants