Skip to content
119 changes: 89 additions & 30 deletions lib/widgets/share.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ import '../log.dart';
import '../model/binding.dart';
import '../model/narrow.dart';
import 'app.dart';
import 'color.dart';
import 'compose_box.dart';
import 'content.dart';
import 'dialog.dart';
import 'icons.dart';
import 'message_list.dart';
import 'page.dart';
import 'recent_dm_conversations.dart';
import 'store.dart';
import 'subscription_list.dart';
import 'text.dart';
import 'theme.dart';

// Responds to receiving shared content from other apps.
Expand Down Expand Up @@ -99,16 +100,16 @@ class ShareService {
mimeType: mimeType);
});

unawaited(navigator.push(
SharePage.buildRoute(
accountId: accountId,
sharedFiles: sharedFiles,
sharedText: intentSendEvent.extraText)));
ShareDialog.show(
pageContext: context,
initialAccountId: accountId,
sharedFiles: sharedFiles,
sharedText: intentSendEvent.extraText);
Comment on lines +107 to +111
Copy link
Collaborator

Choose a reason for hiding this comment

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

share: Re-design from page to a dialog

This commit breaks the "New DM" button in the "Direct messages" tab, because a PageRoot ancestor is no longer found when the button is tapped:

======== Exception caught by gesture ===============================================================
The following assertion was thrown while handling a gesture:
No PageRoot ancestor
'package:zulip/widgets/page.dart':
Failed assertion: line 26 pos 12: 'element != null'

When the exception was thrown, this was the stack: 
#2      PageRoot.contextOf (package:zulip/widgets/page.dart:26:12)
#3      showNewDmSheet (package:zulip/widgets/new_dm_sheet.dart:17:32)
#4      _NewDmButtonState.build.<anonymous closure> (package:zulip/widgets/recent_dm_conversations.dart:270:20)

[etc.]

}
}

class SharePage extends StatelessWidget {
const SharePage({
class ShareDialog extends StatelessWidget {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's link to the Figma in a dartdoc, to help inform future changes to this.

const ShareDialog({
super.key,
required this.sharedFiles,
required this.sharedText,
Expand All @@ -117,16 +118,27 @@ class SharePage extends StatelessWidget {
final Iterable<FileToUpload>? sharedFiles;
final String? sharedText;

static AccountRoute<void> buildRoute({
required int accountId,
static void show({
required BuildContext pageContext,
required int initialAccountId,
required Iterable<FileToUpload>? sharedFiles,
required String? sharedText,
}) {
return MaterialAccountWidgetRoute(
accountId: accountId,
page: SharePage(
sharedFiles: sharedFiles,
sharedText: sharedText));
}) async {
unawaited(showModalBottomSheet<void>(
context: pageContext,
// 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,
useSafeArea: true,
isScrollControlled: true,
builder: (_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
builder: (_) {
// The Figma uses designVariables.mainBackground, which we could set
// here with backgroundColor. Shrug; instead, accept the background color
// from BottomSheetThemeData, which is similar to that (as of 2025-10-07),
// for consistency with other bottom sheets.
builder: (_) {

return PerAccountStoreWidget(
accountId: initialAccountId,
child: ShareDialog(
sharedFiles: sharedFiles,
sharedText: sharedText));
}));
}

void _handleNarrowSelect(BuildContext context, Narrow narrow) {
Expand Down Expand Up @@ -171,28 +183,74 @@ class SharePage extends StatelessWidget {

@override
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
final store = PerAccountStoreWidget.of(context);
final designVariables = DesignVariables.of(context);
final zulipLocalizations = ZulipLocalizations.of(context);

// We should already have the `store.realmIcon` after the
// PerAccountStore has completed loading, hence the `!` here.
final realmIconUrl = store.realmUrl.resolveUri(store.realmIcon!);
Comment on lines +195 to +197
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a PerAccountStore is guaranteed to have non-null .realmIcon, then why is it typed as optional? If we can make it required, we should do so, right? (Ditto .realmName.) Then if there's some subtlety in explaining that, we can do it just once where the field is declared, instead of all the places that try to consume it.


final labelStyle = TextStyle(
fontSize: 18,
height: 24 / 18,
letterSpacing: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't letterSpacing: 0 the default, from zulipTypography?

).merge(weightVariableTextStyle(context, wght: 500));

Widget mkLabel(String text) {
return Text(
text,
style: labelStyle,
overflow: TextOverflow.ellipsis,
maxLines: 1);
}

return DefaultTabController(
length: 2,
child: Scaffold(
appBar: AppBar(
title: Text(zulipLocalizations.sharePageTitle),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This UI string isn't used anymore. That feels kind of weird to me; now this UI doesn't have an explicit label saying what its purpose is. Shrug and remove, I guess, because we're being consistent with the Figma?

Copy link
Collaborator

Choose a reason for hiding this comment

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

…Ah, I see it's removed in a different commit. Let's remove it in the commit that removes its last usage.

bottom: TabBar(
indicatorColor: designVariables.icon,
labelColor: designVariables.foreground,
unselectedLabelColor: designVariables.foreground.withFadedAlpha(0.7),
child: Column(children: [
Row(children: [
SizedBox.square(
dimension: 42,
child: Padding(
padding: const EdgeInsets.all(7),
child: RealmContentNetworkImage(realmIconUrl))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following the link to the "company-logo" component in Figma—

https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=421-14693&m=dev

—it looks like this is supposed to have a 4px border radius:

image

Would the AvatarShape widget be appropriate here? Its dartdoc says "A rounded square shape, to wrap an [AvatarImage] or similar.".

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also see an additional 4px horizontal padding in the Figma that's not implemented in this revision:

https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=12853-76635&m=dev

image

Expanded(child: TabBar(
labelStyle: labelStyle,
labelColor: designVariables.iconSelected,
unselectedLabelColor: designVariables.icon,
indicatorWeight: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, two things stand out to me in the dartdoc of indicatorWeight:

  • /// The value of this parameter must be greater than zero.
  • /// If [indicator] is specified […] this property is ignored.

What happens if we don't pass indicatorWeight?

indicator: BoxDecoration(border: Border(
bottom: BorderSide(
color: designVariables.iconSelected,
width: 4.0))),
Comment on lines +241 to +244
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe:

Suggested change
indicator: BoxDecoration(border: Border(
bottom: BorderSide(
color: designVariables.iconSelected,
width: 4.0))),
indicator: UnderlineTabIndicator(
borderSide: BorderSide(
color: designVariables.iconSelected,
width: 4.0)),

; is that equivalent?

indicatorSize: TabBarIndicatorSize.label,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reading the dartdoc for TabBarIndicatorSize.label, this looks like a step away from the Figma:

  /// The tab's bounds are only as wide as the (centered) tab widget itself.
  ///
  /// This value is used to align the tab's label, typically a [Tab]
  /// widget's text or icon, with the selected tab indicator.
  label,

The other enum value looks like a closer match to the Figma:

  /// The tab indicator's bounds are as wide as the space occupied by the tab
  /// in the tab bar: from the right edge of the previous tab to the left edge
  /// of the next tab.
  tab,

The "indicator" in the Figma is 171.5px wide, whether it's on the tab with the longer "Channels" label or the tab with the shorter "DMs" label:

image

If TabBarIndicatorSize.label is actually used to approach the Figma, but in some counterintuitive way, let's include a comment for the next person reading the code.

dividerHeight: 0,
splashFactory: NoSplash.splashFactory,
tabs: [
Tab(text: zulipLocalizations.channelsPageTitle),
Tab(text: zulipLocalizations.recentDmConversationsPageTitle),
SizedBox(
height: 42,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this UI respond to the device text-size setting; could you do some quick manual tests for that? Some suggestions that might help make it nicer, depending on your testing:

  • Pass a TabBar.labelPadding that's smaller than the default (16px horizontal), to give more room for the text as it gets larger. (Might be helpful anyway, for languages where "Direct messages" translates to a longer string?)
  • Remove this 42px explicit height, or have it respond to the text-size setting.
  • Clamp the labels to a max scale factor, while still allowing some growth (see ui: Sweep through UI text, clamping max scale factor for less-important content #1023).

child: Row(
mainAxisAlignment: MainAxisAlignment.center,
spacing: 4,
children: [
Icon(size: 24, ZulipIcons.hash_italic),
Flexible(child: mkLabel(zulipLocalizations.channelsPageTitle)),
])),
SizedBox(
height: 42,
child: Row(
mainAxisAlignment: MainAxisAlignment.center,
spacing: 4,
children: [
Icon(size: 24, ZulipIcons.two_person),
Flexible(child: mkLabel(zulipLocalizations.recentDmConversationsPageTitle)),
])),
Comment on lines +258 to +266
Copy link
Collaborator

Choose a reason for hiding this comment

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

This tab-bar code looks like a good candidate to be pulled out to a helper method or a new private widget, so we don't have to repeat its details for each tab.

])),
body: TabBarView(children: [
]),
Expanded(child: TabBarView(children: [
SubscriptionListPageBody(
showTopicListButtonInActionSheet: false,
hideChannelsIfUserCantSendMessage: true,
allowGoToAllChannels: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This removal looks unintentional.

onChannelSelect: (narrow) => _handleNarrowSelect(context, narrow),
// TODO(#412) add onTopicSelect, Currently when user lands on the
// channel feed page from subscription list page and they tap
Expand All @@ -204,6 +262,7 @@ class SharePage extends StatelessWidget {
RecentDmConversationsPageBody(
hideDmsIfUserCantPost: true,
onDmSelect: (narrow) => _handleNarrowSelect(context, narrow)),
])));
])),
]));
}
}