msglist: Add message action sheet with "Share" button #12

chrisbobbe · 2023-02-23T20:47:17Z

No description provided.

gnprice

Thanks! Very nice. Some comments.

It feels like we're having to put this together out of lower-level pieces than I would expect — like the Flutter Material implementation of these bottom sheets is incomplete, and not just in the need for a Material 3 update (flutter/flutter#111448). But I think the result will be good.

gnprice · 2023-02-24T00:21:21Z

ios/Podfile

+
+flutter_ios_podfile_setup
+
+target 'Runner' do


Can you say a bit about how you produced this first commit?

I tried adding just the pubspec.yaml line and then running flutter pub get and then flutter run -d <an iOS device>. That produced much of the same generated code, but with a few differences, so I wonder if you did something different.

I re-made the commit and wrote a proper commit message.

gnprice · 2023-02-24T00:40:38Z

lib/widgets/action_sheet.dart

+      // TODO: Share raw Markdown, right, not HTML?
+      Share.share(message.content);


Yeah, agreed.

The raw Markdown won't always be ideal, either. But between the two, I'm pretty sure it'll be preferable more of the time.

(Fine to leave it as a TODO for this PR, though.)

gnprice · 2023-02-24T00:47:30Z

lib/widgets/action_sheet.dart

+        child: Wrap(
+          // TODO: choose `runSpacing` / `spacing`, to control spacing between buttons
+
+          children: _messageActionSheetButtons(message)
+        ),


Let's start by having this be a list of menu items — so a column where each item is an icon and then a label. Similar to this example in Material:
https://m3.material.io/components/bottom-sheets/guidelines#bb88001e-5e28-42a5-bda9-95fc67407121

but I think that's also what I usually see in apps.

The flexibility to do more things with the layout will definitely come in handy later: in particular it'll let us offer a row of common reactions. But if the list of distinct actions like "Share" gets long enough that we really need this wrapped layout for it… then that's probably too many regardless of layout, more actions than people are going to want to read through.

For the specific layout for menu items, perhaps try Material's specs for a menu:
https://m3.material.io/components/menus/specs#6928c7b9-2c6e-4ff6-98a6-55883fb299bd

That seems to be mainly about menus in somewhat different settings, not a bottom sheet, so may not fit exactly. But probably a good starting point.

gnprice · 2023-02-24T01:07:14Z

lib/widgets/draggable_scrollable_modal_bottom_sheet.dart

+    // Clip.hardEdge looks bad; Clip.antiAliasWithSaveLayer looks pixel-perfect
+    // on my iPhone 13 Pro but is marked as "much slower":
+    //   https://api.flutter.dev/flutter/dart-ui/Clip.html
+    clipBehavior: Clip.antiAlias,


Huh — on my Pixel 5, I can't actually spot a difference regardless of what value I put here. Even Clip.none. (And I even restarted, just in case I was missing something about whether hot reload would work; but still nothing changed.)

This one took some extra steps for me to reproduce. Did you test by adding enough content in the sheet that it became scrollable, then as you scrolled down, watch what happened as the content crossed the curved boundary of the sheet in the corner?

Ahh, I see. No, I didn't — I just looked at the corners of the sheet itself, without anything scrolling into them.

gnprice · 2023-02-24T01:17:35Z

lib/widgets/draggable_scrollable_modal_bottom_sheet.dart

+        // (Things break with `expand: true`, the default; not sure why.)
+        expand: false,


Huh, weird. Specifically, what I see is that the bottom sheet then starts out occupying the whole screen, as if initialChildSize was 1.0 instead of 0.5.

That doesn't seem like what the docs call for. May be a bug. Or maybe it's somehow related to the Stack that sits above it?

Noting in a code comment.

gnprice · 2023-02-24T01:36:31Z

lib/widgets/draggable_scrollable_modal_bottom_sheet.dart

+              child: Column(
+                // Extend DraggableScrollableSheet to full width so the whole
+                // sheet responds to drag/scroll uniformly. There's probably a
+                // better way to do this.
+                crossAxisAlignment: CrossAxisAlignment.stretch,
+
+                children: [
+                  Builder(builder: builder),
+                ],
+              ),


Hmm yeah, I went looking for how to do this and found it somewhat tricky.

Here's a pretty clean solution, though:

Suggested change

child: Column(

// Extend DraggableScrollableSheet to full width so the whole

// sheet responds to drag/scroll uniformly. There's probably a

// better way to do this.

crossAxisAlignment: CrossAxisAlignment.stretch,

children: [

Builder(builder: builder),

],

),

child: FractionallySizedBox(

widthFactor: 1.0,

child: Builder(builder: builder),

),

What that does is force the child to have a width that is the max width the parent allowed, while for the height passing through the same constraints (both min and max) that the parent gave.

gnprice · 2023-02-24T01:55:05Z

lib/widgets/draggable_scrollable_modal_bottom_sheet.dart

+    return DraggableScrollableSheet(
+        // Enough less than 1 as to not extend into the device's top inset
+        maxChildSize: 0.75,


If you open the sheet, then drag it lightly down, it just moves down with your finger and then stops there, still open. That feels odd — it should go all the way closed, I think.

It looks like one can fix this by setting initialChildSize slightly larger than minChildSize.

"Slightly", because if you set them equal then there's some odd other behavior: you try to drag it closed, and get the glowing overscroll indicator. (At least on Android.) But e.g. initialChildSize: 0.26, with the default minChildSize of 0.25, fixes that.

Here's a related upstream issue, to watch out for when setting these DraggableScrollableSheet parameters:
flutter/flutter#116982

There's also this messy thread with people describing several different issues:
flutter/flutter#36283 (comment)

Yeah, while I was experimenting with snap: true, ISTR noticing a behavior like the one described in flutter/flutter#116982.

chrisbobbe · 2023-02-25T01:40:37Z

Thanks for the review! Revision pushed.

chrisbobbe · 2023-02-25T01:45:20Z

Ah, not ready to merge actually (so I'll convert to a draft): there's still that issue with showModalBottomSheet's useSafeArea param we discussed in the office today. I plan to send an issue/PR upstream on Monday (edit: issue flutter/flutter#121554; PR flutter/flutter#121588).

chrisbobbe · 2023-03-08T21:09:30Z

With flutter/flutter#121554 resolved by my PR flutter/flutter#122118 (merged today 🎉), our action sheet now behaves well with useSafeArea: true! Revision pushed, and I'm un-marking as draft.

chrisbobbe · 2023-03-08T21:14:40Z

(Just rebased and resolved a conflict.)

gnprice

Thanks! Looks good. One comment-comment below, and: it'd be good to bump the Flutter version constraint, so that the pubspec calls for a version with that safe-area fix.

gnprice · 2023-03-08T22:31:54Z

lib/widgets/action_sheet.dart

+              // TODO: to support iPads, we're asked to give a
+              // `sharePositionOrigin` param, or risk crashing / hanging:
+              //   https://pub.dev/packages/share_plus#ipad


Wacky. That is definitely below the quality standards that would apply to a first-party plugin.

Upstream threads:
fluttercommunity/plus_plugins#870 (comment)
fluttercommunity/plus_plugins#305 (comment)

And it sounds like the old first-party plugin it replaced didn't have this problem:
fluttercommunity/plus_plugins#305 (comment)

Well, the TODO is good enough for merging for the prototype.

gnprice · 2023-03-08T22:38:25Z

lib/widgets/draggable_scrollable_modal_bottom_sheet.dart

+        minChildSize: 0.25,
+        initialChildSize: 0.26,


Let's leave a comment pointing to #12 (comment) for why we're setting one of these to epsilon more than the other.

chrisbobbe · 2023-03-09T21:01:00Z

it'd be good to bump the Flutter version constraint, so that the pubspec calls for a version with that safe-area fix.

Agreed. From the pubspec.yaml file, it looks like the current constraint is >=3.8.0-17.0.pre.49. I think we'll want to wait for a new named version to be released (one that has my PR), and we can just replace the current name there with the new one?

chrisbobbe · 2023-03-09T21:03:26Z

The merged commit was flutter/flutter@23b7bbd89, and my plan is to wait for a new version tag to appear in the GitHub UI in the list of refs that include my commit:

chrisbobbe · 2023-03-10T22:07:40Z

Thanks for the review! Revision pushed, and I think you were going to test on Linux desktop?

This gets us flutter/flutter@23b7bbd89 (flutter/flutter#122118), which we'll use to make a nice prototype for the message action sheet, coming soon.

This plugin seems to work fine so far. When using its main API, `Share.share()`, on iOS, I get some log output that could be noise, and also some that might hint at some layout glitches but that I haven't noticed as being user-visible: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20.60share_plus.60/near/1506606 Here's what I did to install: - Added the new dependency line to pubspec.yaml - Ran `flutter pub get` - Tried to build/run on my iPhone, and in the `pod install` phase, got this error: [!] Automatically assigning platform `iOS` with version `11.0` on target `Runner` because no platform was specified. Please specify a platform for this target in your Podfile. See `https://guides.cocoapods.org/syntax/podfile.html#platform` and fixed by uncommenting the `platform` line in ios/Podfile - Tried to build/run on my iPhone, and in the `pod install` phase, got a different error: [!] Unable to find a target named `RunnerTests` in project `Runner.xcodeproj`, did find `Runner`. and fixed by removing a `target 'RunnerTests' do` block in the iOS and macOS Podfiles. Those blocks were added because of flutter/flutter@2a502363e, which we recently took when upgrading to the Flutter beta 3.8.0-10.1.pre. We were missing new iOS code that those blocks refer to, added in that same Flutter commit. There doesn't seem to be an automatic step that would give us that code, but docs are in the works if we ever want to add it manually: flutter/flutter#96421 (comment) Anyway, it doesn't seem like a lot of code; probably a simple "Hello World" example. Leave it out for now, but cleanly, by deleting those dangling references in the Podfiles. - Built and ran successfully on my iPhone; the `pod install` phase succeeded, generating ios/Podfile.lock and adding reasonable-looking lines in some Xcode files in ios/ (CocoaPods stuff). - (Same with macOS, for the files in macos/.)

chrisbobbe · 2023-03-10T23:26:55Z

Revision pushed, with a TODO mentioning the just-filed #24.

gnprice · 2023-03-10T23:27:38Z

Thanks! Looks good now; merging.

(I tested on Linux, and the UI all works fine. The sharing feature doesn't seem very effective, but I'm not sure what it could usefully do instead.)

chrisbobbe requested a review from gnprice February 23, 2023 20:47

gnprice reviewed Feb 24, 2023

View reviewed changes

chrisbobbe mentioned this pull request Feb 24, 2023

Update Material 3 bottom sheet flutter/flutter#120855

Closed

8 tasks

chrisbobbe force-pushed the pr-action-sheet-share branch from 625fbd0 to 9f91af8 Compare February 25, 2023 01:40

chrisbobbe marked this pull request as draft February 25, 2023 01:45

chrisbobbe force-pushed the pr-action-sheet-share branch from 9f91af8 to 7fcbe99 Compare March 8, 2023 21:06

chrisbobbe marked this pull request as ready for review March 8, 2023 21:09

chrisbobbe force-pushed the pr-action-sheet-share branch from 7fcbe99 to cb776b8 Compare March 8, 2023 21:14

gnprice approved these changes Mar 8, 2023

View reviewed changes

chrisbobbe force-pushed the pr-action-sheet-share branch from cb776b8 to 17b508d Compare March 10, 2023 22:07

chrisbobbe added 2 commits March 10, 2023 14:57

deps: Bump Flutter / Dart SDK floors to latest

84ebc01

This gets us flutter/flutter@23b7bbd89 (flutter/flutter#122118), which we'll use to make a nice prototype for the message action sheet, coming soon.

chrisbobbe force-pushed the pr-action-sheet-share branch from 17b508d to 182cdf9 Compare March 10, 2023 22:59

chrisbobbe mentioned this pull request Mar 10, 2023

ios: Share sheet partly hidden by keyboard, if keyboard was up when message was long-pressed #24

Closed

msglist: Prototype message action sheet with "Share" button

64c7670

chrisbobbe force-pushed the pr-action-sheet-share branch from 182cdf9 to 64c7670 Compare March 10, 2023 23:24

gnprice merged commit 64c7670 into zulip:main Mar 10, 2023

chrisbobbe deleted the pr-action-sheet-share branch March 10, 2023 23:30

gnprice mentioned this pull request Aug 12, 2024

action_sheet: Redesign bottom sheet #853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist: Add message action sheet with "Share" button #12

msglist: Add message action sheet with "Share" button #12

chrisbobbe commented Feb 23, 2023

gnprice left a comment

gnprice Feb 24, 2023

chrisbobbe Feb 25, 2023

gnprice Feb 24, 2023

gnprice Feb 24, 2023

gnprice Feb 24, 2023

gnprice Feb 24, 2023

chrisbobbe Feb 24, 2023 •

edited

Loading

gnprice Feb 24, 2023

gnprice Feb 24, 2023

chrisbobbe Feb 24, 2023

gnprice Feb 24, 2023

gnprice Feb 24, 2023

gnprice Feb 24, 2023

gnprice Feb 24, 2023

chrisbobbe Feb 24, 2023

chrisbobbe commented Feb 25, 2023

chrisbobbe commented Feb 25, 2023 •

edited

Loading

chrisbobbe commented Mar 8, 2023

chrisbobbe commented Mar 8, 2023

gnprice left a comment

gnprice Mar 8, 2023

gnprice Mar 8, 2023

chrisbobbe commented Mar 9, 2023 •

edited

Loading

chrisbobbe commented Mar 9, 2023

chrisbobbe commented Mar 10, 2023

chrisbobbe commented Mar 10, 2023

gnprice commented Mar 10, 2023

		// TODO: Share raw Markdown, right, not HTML?
		Share.share(message.content);

		// (Things break with `expand: true`, the default; not sure why.)
		expand: false,

msglist: Add message action sheet with "Share" button #12

msglist: Add message action sheet with "Share" button #12

Conversation

chrisbobbe commented Feb 23, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Feb 25, 2023

chrisbobbe commented Feb 25, 2023 • edited Loading

chrisbobbe commented Mar 8, 2023

chrisbobbe commented Mar 8, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 9, 2023 • edited Loading

chrisbobbe commented Mar 9, 2023

chrisbobbe commented Mar 10, 2023

chrisbobbe commented Mar 10, 2023

gnprice commented Mar 10, 2023

chrisbobbe Feb 24, 2023 •

edited

Loading

chrisbobbe commented Feb 25, 2023 •

edited

Loading

chrisbobbe commented Mar 9, 2023 •

edited

Loading