diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 59be92482f..30bbf433f5 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -35,11 +35,20 @@ import 'text.dart'; import 'theme.dart'; import 'topic_list.dart'; +/// Show an action sheet with scrollable menu buttons +/// and an optional scrollable header. +/// +/// [header] should not use vertical padding to position itself on the sheet. +/// It will be wrapped in vertical padding +/// and, if [headerScrollable], a scroll view and an [InsetShadowBox]. void _showActionSheet( BuildContext pageContext, { Widget? header, + bool headerScrollable = true, required List> buttonSections, }) { + assert(header is! BottomSheetHeader || !header.outerVerticalPadding); + // Could omit this if we need _showActionSheet outside a per-account context. final accountId = PerAccountStoreWidget.accountIdOf(pageContext); @@ -53,6 +62,52 @@ void _showActionSheet( isScrollControlled: true, builder: (BuildContext _) { final designVariables = DesignVariables.of(pageContext); + + Widget? effectiveHeader; + if (header != null) { + effectiveHeader = headerScrollable + ? Flexible( + // TODO(upstream) Enforce a flex ratio (e.g. 1:3) + // only when the header height plus the buttons' height + // exceeds available space. Otherwise let one or the other + // grow to fill available space even if it breaks the ratio. + // Needs support for separate properties like `flex-grow` + // and `flex-shrink`. + flex: 1, + child: InsetShadowBox( + top: 8, bottom: 8, + color: designVariables.bgContextMenu, + child: SingleChildScrollView( + padding: EdgeInsets.symmetric(vertical: 8), + child: header))) + : Padding( + padding: EdgeInsets.only(top: 16, bottom: 4), + child: header); + } + + final body = Flexible( + flex: (effectiveHeader != null && headerScrollable) + ? 3 + : 1, + child: Padding( + padding: const EdgeInsets.fromLTRB(16, 0, 16, 0), + child: Column( + crossAxisAlignment: CrossAxisAlignment.stretch, + mainAxisSize: MainAxisSize.min, + children: [ + Flexible(child: InsetShadowBox( + top: 8, bottom: 8, + color: designVariables.bgContextMenu, + child: SingleChildScrollView( + padding: const EdgeInsets.symmetric(vertical: 8), + child: Column( + mainAxisSize: MainAxisSize.min, + spacing: 8, + children: buttonSections.map((buttons) => + MenuButtonsShape(buttons: buttons)).toList())))), + const BottomSheetDismissButton(style: BottomSheetDismissButtonStyle.cancel), + ]))); + return PerAccountStoreWidget( accountId: accountId, child: Semantics( @@ -62,43 +117,11 @@ void _showActionSheet( child: Column( mainAxisSize: MainAxisSize.min, children: [ - if (header != null) - Flexible( - // TODO(upstream) Enforce a flex ratio (e.g. 1:3) - // only when the header height plus the buttons' height - // exceeds available space. Otherwise let one or the other - // grow to fill available space even if it breaks the ratio. - // Needs support for separate properties like `flex-grow` - // and `flex-shrink`. - flex: 1, - child: InsetShadowBox( - top: 8, bottom: 8, - color: designVariables.bgContextMenu, - child: SingleChildScrollView( - padding: EdgeInsets.symmetric(vertical: 8), - child: header))) + if (effectiveHeader != null) + effectiveHeader else SizedBox(height: 8), - Flexible( - flex: 3, - child: Padding( - padding: const EdgeInsets.fromLTRB(16, 0, 16, 0), - child: Column( - crossAxisAlignment: CrossAxisAlignment.stretch, - mainAxisSize: MainAxisSize.min, - children: [ - Flexible(child: InsetShadowBox( - top: 8, bottom: 8, - color: designVariables.bgContextMenu, - child: SingleChildScrollView( - padding: const EdgeInsets.symmetric(vertical: 8), - child: Column( - mainAxisSize: MainAxisSize.min, - spacing: 8, - children: buttonSections.map((buttons) => - MenuButtonsShape(buttons: buttons)).toList())))), - const BottomSheetDismissButton(style: BottomSheetDismissButtonStyle.cancel), - ]))), + body, ])))); }); } @@ -114,7 +137,8 @@ typedef WidgetBuilderFromTextStyle = Widget Function(TextStyle); /// The "build" params support richer content, such as [TextWithLink], /// and the callback is passed a [TextStyle] which is the base style. /// -/// Assumes 8px padding below the top of the bottom sheet. +/// To add outer vertical padding to position the header on the sheet, +/// pass true for [outerVerticalPadding]. /// /// Figma; just message no title: /// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3481-26993&m=dev @@ -133,6 +157,7 @@ class BottomSheetHeader extends StatelessWidget { this.buildTitle, this.message, this.buildMessage, + this.outerVerticalPadding = false, }) : assert(message == null || buildMessage == null), assert(title == null || buildTitle == null), assert((message != null || buildMessage != null) @@ -142,6 +167,7 @@ class BottomSheetHeader extends StatelessWidget { final Widget Function(TextStyle)? buildTitle; final String? message; final Widget Function(TextStyle)? buildMessage; + final bool outerVerticalPadding; @override Widget build(BuildContext context) { @@ -149,16 +175,30 @@ class BottomSheetHeader extends StatelessWidget { final baseTitleStyle = TextStyle( fontSize: 20, - height: 20 / 20, + // More height than in Figma, but it was looking too tight: + // https://github.com/zulip/zulip-flutter/pull/1877#issuecomment-3379664807 + // (See use of TextHeightBehavior below.) + height: 24 / 20, color: designVariables.title, ).merge(weightVariableTextStyle(context, wght: 600)); - final effectiveTitle = switch ((buildTitle, title)) { + Widget? effectiveTitle = switch ((buildTitle, title)) { (final build?, null) => build(baseTitleStyle), (null, final data?) => Text(style: baseTitleStyle, data), _ => null, }; + if (effectiveTitle != null) { + effectiveTitle = DefaultTextHeightBehavior( + textHeightBehavior: TextHeightBehavior( + // We want some breathing room between lines, + // without adding margin above or below the title. + applyHeightToFirstAscent: false, + applyHeightToLastDescent: false, + ), + child: effectiveTitle); + } + final baseMessageStyle = TextStyle( color: designVariables.labelTime, fontSize: 17, @@ -170,12 +210,20 @@ class BottomSheetHeader extends StatelessWidget { _ => null, }; - return Padding( - padding: EdgeInsets.fromLTRB(16, 8, 16, 4), + Widget result = Padding( + padding: EdgeInsets.symmetric(horizontal: 16), child: Column( crossAxisAlignment: CrossAxisAlignment.stretch, spacing: 8, children: [?effectiveTitle, ?effectiveMessage])); + + if (outerVerticalPadding) { + result = Padding( + padding: EdgeInsets.only(top: 16, bottom: 4), + child: result); + } + + return result; } } @@ -468,7 +516,21 @@ void showChannelActionSheet(BuildContext context, { [UnsubscribeButton(pageContext: pageContext, channelId: channelId)], ]; - _showActionSheet(pageContext, buttonSections: buttonSections); + final header = BottomSheetHeader( + buildTitle: (baseStyle) => Text.rich( + style: baseStyle, + channelTopicLabelSpan( + context: context, + channelId: channelId, + fontSize: baseStyle.fontSize!, + color: baseStyle.color!)), + // TODO(#1896) show channel description + ); + + _showActionSheet(pageContext, + header: header, + headerScrollable: false, + buttonSections: buttonSections); } class SubscribeButton extends ActionSheetMenuItemButton { @@ -723,7 +785,20 @@ void showTopicActionSheet(BuildContext context, { narrow: TopicNarrow(channelId, topic, with_: someMessageIdInTopic), pageContext: context)); - _showActionSheet(pageContext, buttonSections: [optionButtons]); + final header = BottomSheetHeader( + buildTitle: (baseStyle) => Text.rich( + style: baseStyle, + channelTopicLabelSpan( + context: context, + channelId: channelId, + topic: topic, + fontSize: baseStyle.fontSize!, + color: baseStyle.color!))); + + _showActionSheet(pageContext, + header: header, + headerScrollable: false, + buttonSections: [optionButtons]); } class UserTopicUpdateButton extends ActionSheetMenuItemButton { diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index 2c7c5d61ab..45131d7ad0 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -804,10 +804,9 @@ class ViewReactionsHeader extends StatelessWidget { final reactions = message?.reactions; if (reactions == null || reactions.aggregated.isEmpty) { - return Padding( - padding: const EdgeInsets.only(top: 8), - child: BottomSheetHeader(message: zulipLocalizations.seeWhoReactedSheetNoReactions), - ); + return BottomSheetHeader( + outerVerticalPadding: true, + message: zulipLocalizations.seeWhoReactedSheetNoReactions); } return Padding( diff --git a/lib/widgets/read_receipts.dart b/lib/widgets/read_receipts.dart index 46c920372a..8416d78c85 100644 --- a/lib/widgets/read_receipts.dart +++ b/lib/widgets/read_receipts.dart @@ -128,11 +128,10 @@ class _ReadReceiptsHeader extends StatelessWidget { markup: zulipLocalizations.actionSheetReadReceiptsReadCount(receiptCount)); } - return Padding( - padding: const EdgeInsets.only(top: 8), - child: BottomSheetHeader( - title: zulipLocalizations.actionSheetReadReceipts, - buildMessage: headerMessageBuilder)); + return BottomSheetHeader( + outerVerticalPadding: true, + title: zulipLocalizations.actionSheetReadReceipts, + buildMessage: headerMessageBuilder); } } diff --git a/lib/widgets/text.dart b/lib/widgets/text.dart index 3fe48484e3..10a6485023 100644 --- a/lib/widgets/text.dart +++ b/lib/widgets/text.dart @@ -4,6 +4,10 @@ import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; +import '../api/model/model.dart'; +import '../generated/l10n/zulip_localizations.dart'; +import 'icons.dart'; +import 'store.dart'; import 'theme.dart'; /// An app-wide [Typography] for Zulip, customized from the Material default. @@ -533,3 +537,161 @@ class _TextWithLinkState extends State { span); } } + +/// Data to size and position a square icon in a span of text. +class InlineIconGeometryData { + /// What size the icon should be, + /// as a fraction of the surrounding text's font size. + final double sizeFactor; + + /// Where to assign the icon's baseline, as a fraction of the icon's size, + /// when the span is rendered with [TextBaseline.alphabetic]. + /// + /// This is ignored when the span is rendered with [TextBaseline.ideographic]; + /// zero is used instead. + final double alphabeticBaselineFactor; + + /// How much horizontal padding should separate the icon from surrounding text, + /// as a fraction of the icon's size. + final double paddingFactor; + + const InlineIconGeometryData._({ + required this.sizeFactor, + required this.alphabeticBaselineFactor, + required this.paddingFactor, + }); + + factory InlineIconGeometryData.forIcon(IconData icon) { + final result = _inlineIconGeometries[icon]; + assert(result != null); + return result ?? _defaultGeometry; + } + + // Values are ad hoc unless otherwise specified. + static final Map _inlineIconGeometries = { + ZulipIcons.globe: InlineIconGeometryData._( + sizeFactor: 0.8, + alphabeticBaselineFactor: 1 / 8, + paddingFactor: 1 / 4), + + ZulipIcons.hash_sign: InlineIconGeometryData._( + sizeFactor: 0.8, + alphabeticBaselineFactor: 1 / 16, + paddingFactor: 1 / 4), + + ZulipIcons.lock: InlineIconGeometryData._( + sizeFactor: 0.8, + alphabeticBaselineFactor: 1 / 16, + paddingFactor: 1 / 4), + + ZulipIcons.chevron_right: InlineIconGeometryData._( + sizeFactor: 1, + alphabeticBaselineFactor: 5 / 24, + paddingFactor: 0), + }; + + static final _defaultGeometry = InlineIconGeometryData._( + sizeFactor: 0.8, + alphabeticBaselineFactor: 1 / 16, + paddingFactor: 1 / 4, + ); +} + +/// An icon, sized and aligned for use in a span of text. +WidgetSpan iconWidgetSpan({ + required IconData icon, + required double fontSize, + required TextBaseline baselineType, + required Color? color, + bool padBefore = false, + bool padAfter = false, +}) { + final InlineIconGeometryData( + :sizeFactor, + :alphabeticBaselineFactor, + :paddingFactor, + ) = InlineIconGeometryData.forIcon(icon); + + final size = sizeFactor * fontSize; + + final effectiveBaselineOffset = switch (baselineType) { + TextBaseline.alphabetic => alphabeticBaselineFactor * size, + TextBaseline.ideographic => 0.0, + }; + + Widget child = Icon(size: size, color: color, icon); + + if (effectiveBaselineOffset != 0) { + child = Transform.translate( + offset: Offset(0, effectiveBaselineOffset), + child: child); + } + + if (padBefore || padAfter) { + final padding = paddingFactor * size; + child = Padding( + padding: EdgeInsetsDirectional.only( + start: padBefore ? padding : 0, + end: padAfter ? padding : 0, + ), + child: child); + } + + return WidgetSpan( + alignment: PlaceholderAlignment.baseline, + baseline: baselineType, + child: child); +} + +/// An [InlineSpan] with a channel privacy icon, channel name, +/// and optionally a chevron-right icon plus topic. +/// +/// Pass this to [Text.rich], which can be styled arbitrarily. +/// Pass the [fontSize] and [color] of surrounding text +/// so that the icons are sized and colored appropriately. +InlineSpan channelTopicLabelSpan({ + required BuildContext context, + required int channelId, + TopicName? topic, + required double fontSize, + required Color color, +}) { + final zulipLocalizations = ZulipLocalizations.of(context); + final store = PerAccountStoreWidget.of(context); + final channel = store.streams[channelId]; + final subscription = store.subscriptions[channelId]; + final swatch = colorSwatchFor(context, subscription); + final channelIcon = channel != null ? iconDataForStream(channel) : null; + final baselineType = localizedTextBaseline(context); + + return TextSpan(children: [ + if (channelIcon != null) + iconWidgetSpan( + icon: channelIcon, + fontSize: fontSize, + baselineType: baselineType, + color: swatch.iconOnPlainBackground, + padAfter: true), + if (channel != null) + TextSpan(text: channel.name) + else + TextSpan( + style: TextStyle(fontStyle: FontStyle.italic), + text: zulipLocalizations.unknownChannelName), + if (topic != null) ...[ + iconWidgetSpan( + icon: ZulipIcons.chevron_right, + fontSize: fontSize, + baselineType: baselineType, + color: color, + padBefore: true, + padAfter: true), + if (topic.displayName != null) + TextSpan(text: topic.displayName) + else + TextSpan( + style: TextStyle(fontStyle: FontStyle.italic), + text: store.realmEmptyTopicDisplayName), + ], + ]); +} diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index a7db054b55..5cb6df7c24 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -54,6 +54,9 @@ extension SavedSnippetChecks on Subject { extension ZulipStreamChecks on Subject { Subject get streamId => has((x) => x.streamId, 'streamId'); + + Subject get inviteOnly => has((x) => x.inviteOnly, 'inviteOnly'); + Subject get isWebPublic => has((x) => x.isWebPublic, 'isWebPublic'); } extension ChannelFolderChecks on Subject { diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 06e0eab5ca..621f759f28 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -38,6 +38,7 @@ import 'package:zulip/widgets/topic_list.dart'; import 'package:zulip/widgets/user.dart'; import '../api/fake_api.dart'; +import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; @@ -255,19 +256,21 @@ void main() { await tester.pump(const Duration(milliseconds: 250)); } - Future showFromTopicListAppBar(WidgetTester tester) async { + Future showFromTopicListAppBar(WidgetTester tester, {int? streamId}) async { + streamId ??= someChannel.streamId; final transitionDurationObserver = TransitionDurationObserver(); connection.prepare(json: GetStreamTopicsResult(topics: []).toJson()); await tester.pumpWidget(TestZulipApp( navigatorObservers: [transitionDurationObserver], accountId: eg.selfAccount.id, - child: TopicListPage(streamId: someChannel.streamId))); + child: TopicListPage(streamId: streamId))); await tester.pump(); + final titleText = store.streams[streamId]?.name ?? '(unknown channel)'; await tester.longPress(find.descendant( of: find.byType(ZulipAppBar), - matching: find.text(someChannel.name))); + matching: find.text(titleText))); await transitionDurationObserver.pumpPastTransition(tester); } @@ -290,6 +293,61 @@ void main() { checkButton('Copy link to channel'); } + group('header', () { + final findHeader = find.descendant( + of: actionSheetFinder, + matching: find.byType(BottomSheetHeader)); + + Finder findInHeader(Finder finder) => + find.descendant(of: findHeader, matching: finder); + + testWidgets('public channel', (tester) async { + await prepare(); + check(store.streams[someChannel.streamId]).isNotNull() + ..inviteOnly.isFalse()..isWebPublic.isFalse(); + await showFromInbox(tester); + check(findInHeader(find.byIcon(ZulipIcons.hash_sign))).findsOne(); + check(findInHeader(find.textContaining(someChannel.name))).findsOne(); + }); + + testWidgets('web-public channel', (tester) async { + await prepare(); + await store.handleEvent(ChannelUpdateEvent(id: 1, + streamId: someChannel.streamId, + name: someChannel.name, + property: null, value: null, + // (Ideally we'd use `property` and `value` but I'm not sure if + // modern servers actually do that or if they still use this + // separate field.) + isWebPublic: true)); + check(store.streams[someChannel.streamId]).isNotNull() + ..inviteOnly.isFalse()..isWebPublic.isTrue(); + await showFromInbox(tester); + check(findInHeader(find.byIcon(ZulipIcons.globe))).findsOne(); + check(findInHeader(find.textContaining(someChannel.name))).findsOne(); + }); + + testWidgets('private channel', (tester) async { + await prepare(); + await store.handleEvent(eg.channelUpdateEvent(someChannel, + property: ChannelPropertyName.inviteOnly, value: true)); + check(store.streams[someChannel.streamId]).isNotNull() + ..inviteOnly.isTrue()..isWebPublic.isFalse(); + await showFromInbox(tester); + check(findInHeader(find.byIcon(ZulipIcons.lock))).findsOne(); + check(findInHeader(find.textContaining(someChannel.name))).findsOne(); + }); + + testWidgets('unknown channel', (tester) async { + await prepare(); + await store.handleEvent(ChannelDeleteEvent(id: 1, streams: [someChannel])); + check(store.streams[someChannel.streamId]).isNull(); + await showFromTopicListAppBar(tester); + check(findInHeader(find.byType(Icon))).findsNothing(); + check(findInHeader(find.textContaining('(unknown channel)'))).findsOne(); + }); + }); + testWidgets('show from inbox', (tester) async { await prepare(); await showFromInbox(tester); @@ -747,6 +805,38 @@ void main() { checkButton('Copy link to topic'); } + group('header', () { + final findHeader = find.descendant( + of: actionSheetFinder, + matching: find.byType(BottomSheetHeader)); + + Finder findInHeader(Finder finder) => + find.descendant(of: findHeader, matching: finder); + + testWidgets('with topic', (tester) async { + await prepare(); + check(store.streams[someChannel.streamId]).isNotNull() + ..inviteOnly.isFalse()..isWebPublic.isFalse(); + await showFromAppBar(tester); + check(findInHeader(find.byIcon(ZulipIcons.hash_sign))).findsOne(); + check(findInHeader(find.textContaining(someChannel.name))).findsOne(); + check(findInHeader(find.textContaining(someTopic))).findsOne(); + }); + + testWidgets('without topic (general chat)', (tester) async { + await prepare(topic: ''); + check(store.streams[someChannel.streamId]).isNotNull() + ..inviteOnly.isFalse()..isWebPublic.isFalse(); + final message = eg.streamMessage( + stream: someChannel, topic: '', sender: eg.otherUser); + await showFromAppBar(tester, messages: [message], topic: eg.t('')); + check(findInHeader(find.byIcon(ZulipIcons.hash_sign))).findsOne(); + check(findInHeader(find.textContaining(someChannel.name))).findsOne(); + check(findInHeader(find.textContaining(store.realmEmptyTopicDisplayName))) + .findsOne(); + }); + }); + testWidgets('show from inbox; message in Unreads but not in MessageStore', (tester) async { await prepare(unreadMsgs: eg.unreadMsgs(count: 1, channels: [eg.unreadChannelMsgs(