Redesigned stream/topic recipient headers #416
There are no files selected for viewing
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import 'dart:math'; | ||
| import 'dart:ui'; | ||
|
|
||
| import 'package:collection/collection.dart'; | ||
| import 'package:flutter/material.dart'; | ||
|
|
@@ -46,13 +47,30 @@ class MessageListPage extends StatefulWidget { | |
| State<MessageListPage> createState() => _MessageListPageState(); | ||
| } | ||
|
|
||
| const _kFallbackStreamColor = Color(0xfff5f5f5); | ||
|
|
||
| class _MessageListPageState extends State<MessageListPage> { | ||
| final GlobalKey<ComposeBoxController> _composeBoxKey = GlobalKey(); | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
|
|
||
| final Color backgroundColor; | ||
| switch(widget.narrow) { | ||
| case AllMessagesNarrow(): | ||
| backgroundColor = _kFallbackStreamColor; | ||
| case StreamNarrow(:final streamId): | ||
| case TopicNarrow(:final streamId): | ||
| backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground | ||
| ?? _kFallbackStreamColor; | ||
| case DmNarrow(): | ||
| backgroundColor = _kFallbackStreamColor; | ||
| } | ||
|
|
||
| return Scaffold( | ||
| appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow), | ||
| backgroundColor: backgroundColor), | ||
| // TODO question for Vlad: for a stream view, should we set | ||
| // [backgroundColor] based on stream color, as in this frame: | ||
| // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9684&mode=dev | ||
|
|
@@ -324,12 +342,14 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat | |
| padding: EdgeInsets.symmetric(vertical: 16.0), | ||
| child: CircularProgressIndicator())); // TODO perhaps a different indicator | ||
| case MessageListRecipientHeaderItem(): | ||
| final header = RecipientHeader(message: data.message, narrow: widget.narrow); | ||
| return StickyHeaderItem(allowOverflow: true, | ||
| header: header, child: header); | ||
| case MessageListMessageItem(): | ||
| final header = RecipientHeader(message: data.message, narrow: widget.narrow); | ||
| return MessageItem( | ||
| key: ValueKey(data.message.id), | ||
| header: header, | ||
| trailingWhitespace: i == 1 ? 8 : 11, | ||
| item: data); | ||
| } | ||
|
|
@@ -428,16 +448,17 @@ class MarkAsReadWidget extends StatelessWidget { | |
| } | ||
|
|
||
| class RecipientHeader extends StatelessWidget { | ||
| const RecipientHeader({super.key, required this.message, required this.narrow}); | ||
|
|
||
| final Message message; | ||
| final Narrow narrow; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final message = this.message; | ||
| return switch (message) { | ||
| StreamMessage() => StreamMessageRecipientHeader(message: message, | ||
| showStream: narrow is AllMessagesNarrow), | ||
| DmMessage() => DmRecipientHeader(message: message), | ||
| }; | ||
| } | ||
|
|
@@ -447,18 +468,20 @@ class MessageItem extends StatelessWidget { | |
| const MessageItem({ | ||
| super.key, | ||
| required this.item, | ||
| required this.header, | ||
| this.trailingWhitespace, | ||
| }); | ||
|
|
||
| final MessageListMessageItem item; | ||
| final Widget header; | ||
| final double? trailingWhitespace; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final message = item.message; | ||
| return StickyHeaderItem( | ||
| allowOverflow: !item.isLastInBlock, | ||
| header: header, | ||
| child: _UnreadMarker( | ||
| isRead: message.flags.contains(MessageFlag.read), | ||
| child: ColoredBox( | ||
|
|
@@ -511,60 +534,117 @@ class _UnreadMarker extends StatelessWidget { | |
| } | ||
| } | ||
|
|
||
| class StreamMessageRecipientHeader extends StatelessWidget { | ||
| const StreamMessageRecipientHeader({ | ||
| super.key, | ||
| required this.message, | ||
| required this.showStream, | ||
| }); | ||
|
|
||
| final StreamMessage message; | ||
| final bool showStream; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| // For design specs, see: | ||
| // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev | ||
| // https://github.com/zulip/zulip-mobile/issues/5511 | ||
| final store = PerAccountStoreWidget.of(context); | ||
|
|
||
| final topic = message.subject; | ||
|
|
||
| final subscription = store.subscriptions[message.streamId]; | ||
| final Color backgroundColor; | ||
| final Color contrastingColor; | ||
| final Color iconColor; | ||
| if (subscription != null) { | ||
| final swatch = subscription.colorSwatch(); | ||
| backgroundColor = swatch.barBackground; | ||
| contrastingColor = | ||
| (ThemeData.estimateBrightnessForColor(swatch.barBackground) == Brightness.dark) | ||
| ? Colors.white | ||
| : Colors.black; | ||
| iconColor = swatch.iconOnBarBackground; | ||
| } else { | ||
| backgroundColor = _kFallbackStreamColor; | ||
| contrastingColor = Colors.black; | ||
| iconColor = Colors.black; | ||
| } | ||
| final textStyle = TextStyle( | ||
| color: contrastingColor, | ||
| fontFamily: 'Source Sans 3', | ||
| fontSize: 16, | ||
| letterSpacing: 0.02 * 16, | ||
| height: (18 / 16), | ||
| ).merge(weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900)); | ||
|
|
||
| final Widget streamWidget; | ||
| if (!showStream) { | ||
| streamWidget = const SizedBox(width: 16); | ||
| } else { | ||
| final stream = store.streams[message.streamId]; | ||
| final streamName = stream?.name ?? message.displayRecipient; // TODO(log) if missing | ||
|
|
||
| streamWidget = GestureDetector( | ||
| onTap: () => Navigator.push(context, | ||
| MessageListPage.buildRoute(context: context, | ||
| narrow: StreamNarrow(message.streamId))), | ||
| child: Row( | ||
| crossAxisAlignment: CrossAxisAlignment.center, | ||
| children: [ | ||
| Padding( | ||
| // Figma specifies 5px horizontal spacing around an icon that's | ||
| // 18x18 and includes 1px padding. The icon SVG is flush with | ||
| // the edges, so make it 16x16 with 6px horizontal padding. | ||
| // Bottom padding added here to shift icon up to | ||
| // match alignment with text visually. | ||
| padding: const EdgeInsets.only(left: 6, right: 6, bottom: 3), | ||
| child: Icon(size: 16, color: iconColor, | ||
| // A null [Icon.icon] makes a blank space. | ||
| (stream != null) ? iconDataForStream(stream) : null)), | ||
| Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 11), | ||
| child: Text(streamName, | ||
| style: textStyle, | ||
| overflow: TextOverflow.ellipsis), | ||
| ), | ||
| Padding( | ||
| // Figma has 5px horizontal padding around an 8px wide icon. | ||
| // Icon is 16px wide here so horizontal padding is 1px. | ||
| padding: const EdgeInsets.symmetric(horizontal: 1), | ||
| child: Icon(size: 16, | ||
| color: contrastingColor.withOpacity(0.6), | ||
| ZulipIcons.chevron_right)), | ||
| ])); | ||
| } | ||
|
|
||
| return GestureDetector( | ||
| onTap: () => Navigator.push(context, | ||
| MessageListPage.buildRoute(context: context, | ||
| narrow: TopicNarrow.ofMessage(message))), | ||
| child: ColoredBox( | ||
| color: backgroundColor, | ||
| child: Row( | ||
| crossAxisAlignment: CrossAxisAlignment.center, | ||
| children: [ | ||
| // TODO(#282): Long stream name will break layout; find a fix. | ||
| streamWidget, | ||
| Expanded( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 11), | ||
| child: Text(topic, | ||
| // TODO: Give a way to see the whole topic (maybe a | ||
| // long-press interaction?) | ||
| overflow: TextOverflow.ellipsis, | ||
| style: textStyle))), | ||
| // TODO topic links? | ||
| // Then web also has edit/resolve/mute buttons. Skip those for mobile. | ||
| RecipientHeaderDate(message: message, | ||
| color: contrastingColor.withOpacity(0.4)), | ||
| ]))); | ||
| } | ||
| } | ||
|
|
||
| class DmRecipientHeader extends StatelessWidget { | ||
| const DmRecipientHeader({super.key, required this.message}); | ||
|
|
||
|
|
@@ -599,33 +679,43 @@ class DmRecipientHeader extends StatelessWidget { | |
| color: _kDmRecipientHeaderColor, | ||
| child: Text(style: const TextStyle(color: Colors.white), | ||
| title)), | ||
| RecipientHeaderDate(message: message, | ||
| color: _kRecipientHeaderDateColor), | ||
| ]))); | ||
| } | ||
| } | ||
|
|
||
| final _kDmRecipientHeaderColor = const HSLColor.fromAHSL(1, 0, 0, 0.27).toColor(); | ||
|
|
||
| class RecipientHeaderDate extends StatelessWidget { | ||
| const RecipientHeaderDate({super.key, required this.message, required this.color}); | ||
|
|
||
| final Message message; | ||
| final Color color; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return Padding( | ||
| padding: const EdgeInsets.fromLTRB(10, 0, 16, 0), | ||
| child: Text( | ||
| style: TextStyle( | ||
| color: color, | ||
| fontFamily: 'Source Sans 3', | ||
| fontSize: 16, | ||
| // In Figma this has a line-height of 19, but using 18 | ||
| // here to match the stream/topic text widgets helps | ||
| // to align all the text to the same baseline. | ||
| height: (18 / 16), | ||
|
Comment on lines
+705
to
+708
There was a problem hiding this comment. This lgtm; I believe the line-height doesn't matter here (on the datestamp) except as a means to get it properly vertically aligned, and this seems like a good way to do that. @terpimost FYI. |
||
| // This is equivalent to css `all-small-caps`, see: | ||
| // https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-caps#all-small-caps | ||
| fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], | ||
| ).merge(weightVariableTextStyle(context)), | ||
| _kRecipientHeaderDateFormat.format( | ||
| DateTime.fromMillisecondsSinceEpoch(message.timestamp * 1000)))); | ||
| } | ||
| } | ||
|
|
||
| final _kRecipientHeaderDateColor = const HSLColor.fromAHSL(0.75, 0, 0, 0.15).toColor(); | ||
|
|
||
| final _kRecipientHeaderDateFormat = DateFormat('y-MM-dd', 'en_US'); // TODO(#278) | ||
|
|
||
|
|
@@ -648,7 +738,7 @@ class RecipientHeaderChevronContainer extends StatelessWidget { | |
| decoration: ShapeDecoration(color: color, shape: recipientBorderShape), | ||
| padding: const EdgeInsets.only(right: chevronLength), | ||
| child: Padding( | ||
| padding: const EdgeInsets.fromLTRB(16, 4, 6, 3), child: child)); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -699,7 +789,7 @@ class MessageWithPossibleSender extends StatelessWidget { | |
| ])), | ||
| Container( | ||
| width: 80, | ||
| padding: const EdgeInsets.only(top: 4, right: 16 - 8), | ||
| alignment: Alignment.topRight, | ||
| child: Text(time, style: _kMessageTimestampStyle)), | ||
| ]))); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What's the origin of the number 11 for vertical padding? I'm not finding that in the Figma design.
Also, it seems like it'd be simpler to apply the 11px vertical padding to the entire Row. Is there a reason I'm missing why that wouldn't have the right effect?
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.
This is reverse engineered from a 40px height given the icon height (18p) for this element, and was also applied to the text widget that has 18px line height. The chevron has 16 height and thus given
vertical: 12. I don't think it's clear that wrapping the whole row is an improvement as the chevron would need padding as well.I'll take a closer look at the behavior of cranking up text size in the system settings. In your screenshot it seems that this doesn't affect icon sizes, in which case it might make sense to apply a uniform padding on text elements and vertically center the icons another way.
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.
After our call today I've settled on applying a
CrossAxisAlignment.centerto all the row elements and applying avertical: 11padding to the text elements. I've changed the date line-height to match the text elements so they all visually appear to have the same baseline and let the icons settle on the center of the vertical (having also had to tweak the stream icon a bit).The text elements maintain their alignment even if the system requests they be rendered larger, and the icons don't look terrible where they are placed:
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.
Cool, that solution sounds good to me.
If Flutter's
Rowaccepted an option likeCrossAxisAlignment.baselineCenter, so that it aligned to the baseline but fell back tocenterinstead of tostart, then that would be cleaner because it'd let us get the same effect as you have there but without having to mess with the line-height. But alas.Alternatively if there were a widget that were akin to
IgnoreBaselinebut it moved the baseline around, rather than erase it, then we could useCrossAxisAlignment.baselineand use that widget to adjust where the icons sit vertically. That'd be a slightly different result, which might be better or worse. If that need comes up again, I may write such a widget — it'll require writing a render object, but shouldn't be a lot of code.