Skip to content

msglist: Update layout in messages #446

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 25, 2024
Merged

msglist: Update layout in messages #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45e4ef4
msglist: Adjust text styles in MessageWithPossibleSender
sirpengi Dec 6, 2023
b8c89d1
msglist [nfc]: Move senderRow to local
sirpengi Jan 23, 2024
089c2c9
msglist: Update message layout to new design
sirpengi Jan 12, 2024
97f6678
msglist [nfc]: Move message time definition into conditional it is us…
sirpengi Jan 24, 2024
058b5e4
msglist [nfc]: Group message and sender as Column of Rows, not vice v…
sirpengi Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 62 additions & 43 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,9 @@ String formatHeaderDate(
}

/// A Zulip message, showing the sender's name and avatar if specified.
// Design referenced from:
// - https://github.com/zulip/zulip-mobile/issues/5511
// - https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev
class MessageWithPossibleSender extends StatelessWidget {
const MessageWithPossibleSender({super.key, required this.item});

Expand All @@ -849,61 +852,77 @@ class MessageWithPossibleSender extends StatelessWidget {
@override
Widget build(BuildContext context) {
final message = item.message;
final time = _kMessageTimestampFormat
.format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp));

Widget? senderRow;
if (item.showSender) {
final time = _kMessageTimestampFormat
.format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp));
senderRow = Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: TextBaseline.alphabetic,
children: [
Flexible(
child: GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Row(
children: [
Avatar(size: 32, borderRadius: 3,
userId: message.senderId),
const SizedBox(width: 8),
Flexible(
child: Text(message.senderFullName, // TODO get from user data
style: const TextStyle(
fontFamily: 'Source Sans 3',
fontSize: 18,
height: (22 / 18),
).merge(weightVariableTextStyle(context, wght: 600,
wghtIfPlatformRequestsBold: 900)),
overflow: TextOverflow.ellipsis)),
]))),
const SizedBox(width: 4),
Text(time,
style: TextStyle(
color: _kMessageTimestampColor,
fontFamily: 'Source Sans 3',
fontSize: 16,
height: (18 / 16),
fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')],
).merge(weightVariableTextStyle(context))),
]);
}

return GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () => showMessageActionSheet(context: context, message: message),
// TODO clean up this layout, by less precisely imitating web
child: Padding(
padding: const EdgeInsets.only(top: 2, bottom: 3, left: 8, right: 8),
child: Row(crossAxisAlignment: CrossAxisAlignment.start, children: [
item.showSender
? Padding(
padding: const EdgeInsets.fromLTRB(3, 6, 11, 0),
child: GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Avatar(size: 35, borderRadius: 4,
userId: message.senderId)))
: const SizedBox(width: 3 + 35 + 11),
Expanded(
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
if (item.showSender) ...[
const SizedBox(height: 3),
GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Text(message.senderFullName, // TODO get from user data
style: const TextStyle(fontWeight: FontWeight.bold))),
const SizedBox(height: 4),
],
MessageContent(message: message, content: item.content),
if ((message.reactions?.total ?? 0) > 0)
ReactionChipsList(messageId: message.id, reactions: message.reactions!)
])),
Container(
width: 80,
padding: const EdgeInsets.only(top: 4, right: 16 - 8),
alignment: Alignment.topRight,
child: Text(time, style: _kMessageTimestampStyle)),
padding: const EdgeInsets.symmetric(vertical: 4),
child: Column(children: [
if (senderRow != null)
Padding(padding: const EdgeInsets.fromLTRB(16, 2, 16, 4),
child: senderRow),
Row(crossAxisAlignment: CrossAxisAlignment.start, children: [
const SizedBox(width: 16),
Expanded(
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
MessageContent(message: message, content: item.content),
if ((message.reactions?.total ?? 0) > 0)
ReactionChipsList(messageId: message.id, reactions: message.reactions!)
])),
const SizedBox(width: 16),
]),
])));
}
}

// TODO web seems to ignore locale in formatting time, but we could do better
final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US');

// TODO this seems to come out lighter than on web
final _kMessageTimestampStyle = TextStyle(
fontSize: 12,
fontWeight: FontWeight.w400,
color: const HSLColor.fromAHSL(0.4, 0, 0, 0.2).toColor());
final _kMessageTimestampColor = const HSLColor.fromAHSL(1, 0, 0, 0.5).toColor();

Future<void> markNarrowAsRead(BuildContext context, Narrow narrow) async {
final store = PerAccountStoreWidget.of(context);
Expand Down
6 changes: 3 additions & 3 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ void main() {

testWidgets('basic', (tester) async {
await setupMessageListPage(tester, foundOldest: false,
messages: List.generate(200, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
check(itemCount(tester)).equals(203);
messages: List.generate(300, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
check(itemCount(tester)).equals(303);
Comment on lines -89 to +90
Copy link
Member

Choose a reason for hiding this comment

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

nit in commit message:

msglist: Update message layout to new design

Test 'basic' in group 'fetch older messages on scroll'
adjusted as layout change has caused the test to fail.

That body paragraph can be simplified to read:

Adjusted a test that was broken by the layout change.

i.e. to leave out the name of the test. For a high-level understanding of what the commit is about, the test's name doesn't matter at all — only that it's some test that got broken by the change. And if you're reading in detail, it's redundant with the code.


// Fling-scroll upward...
await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
Expand All @@ -100,7 +100,7 @@ void main() {
await tester.pump(Duration.zero); // Allow a frame for the response to arrive.

// Now we have more messages.
check(itemCount(tester)).equals(303);
check(itemCount(tester)).equals(403);
});

testWidgets('observe double-fetch glitch', (tester) async {
Expand Down