Skip to content

msglist: Add end-of-feed whitespace; use background color from Figma #402

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 4 commits into from
Nov 20, 2023
Merged

msglist: Add end-of-feed whitespace; use background color from Figma #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56ab395
msglist: Add 36px space at bottom, to reinforce that end-of-feed is r…
chrisbobbe Nov 20, 2023
c8fe66e
msglist [nfc]: Give MessageItem the SizedBox for trailing whitespace
chrisbobbe Nov 20, 2023
dff550e
msglist: Move white background color to MessageItem, from whole list
chrisbobbe Nov 20, 2023
3b95070
msglist [nfc]: Move a TODO; note question for Vlad
chrisbobbe Nov 20, 2023
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
127 changes: 67 additions & 60 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ class _MessageListPageState extends State<MessageListPage> {
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow)),
// 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
// That's not obviously preferred over the default background that
// we matched to the Figma in 21dbae120. See another frame, which uses that:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=147%3A9088&mode=dev
body: Builder(
builder: (BuildContext context) => Center(
child: Column(children: [
Expand Down Expand Up @@ -229,35 +235,33 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// TODO figure out text color -- web is supposedly hsl(0deg 0% 20%),
// but seems much darker than that
style: const TextStyle(color: Color.fromRGBO(0, 0, 0, 1)),
child: ColoredBox(
color: Colors.white,
// Pad the left and right insets, for small devices in landscape.
child: SafeArea(
// Don't let this be the place we pad the bottom inset. When there's
// no compose box, we want to let the message-list content pad it.
// TODO(#311) Remove as unnecessary if we do a bottom nav.
// The nav will pad the bottom inset, and an ancestor of this widget
// will have a `MediaQuery.removePadding` with `removeBottom: true`.
bottom: false,

child: Center(
child: ConstrainedBox(
constraints: const BoxConstraints(maxWidth: 760),
child: NotificationListener<ScrollMetricsNotification>(
onNotification: _metricsChanged,
child: Stack(
children: <Widget>[
_buildListView(context),
Positioned(
bottom: 0,
right: 0,
// TODO(#311) SafeArea shouldn't be needed if we have a
// bottom nav. That will pad the bottom inset.
child: SafeArea(
child: ScrollToBottomButton(
scrollController: scrollController,
visibleValue: _scrollToBottomVisibleValue))),
])))))));
// Pad the left and right insets, for small devices in landscape.
child: SafeArea(
// Don't let this be the place we pad the bottom inset. When there's
// no compose box, we want to let the message-list content pad it.
// TODO(#311) Remove as unnecessary if we do a bottom nav.
// The nav will pad the bottom inset, and an ancestor of this widget
// will have a `MediaQuery.removePadding` with `removeBottom: true`.
bottom: false,

child: Center(
child: ConstrainedBox(
constraints: const BoxConstraints(maxWidth: 760),
child: NotificationListener<ScrollMetricsNotification>(
onNotification: _metricsChanged,
child: Stack(
children: <Widget>[
_buildListView(context),
Positioned(
bottom: 0,
right: 0,
// TODO(#311) SafeArea shouldn't be needed if we have a
// bottom nav. That will pad the bottom inset.
child: SafeArea(
child: ScrollToBottomButton(
scrollController: scrollController,
visibleValue: _scrollToBottomVisibleValue))),
]))))));
}

Widget _buildListView(context) {
Expand Down Expand Up @@ -293,10 +297,10 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
final valueKey = key as ValueKey;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - (index - 1);
return length - 1 - (index - 2);
},
controller: scrollController,
itemCount: length + 1,
itemCount: length + 2,
// Setting reverse: true means the scroll starts at the bottom.
// Flipping the indexes (in itemBuilder) means the start/bottom
// has the latest messages.
Expand All @@ -305,9 +309,13 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// TODO on new message when scrolled up, anchor scroll to what's in view
reverse: true,
itemBuilder: (context, i) {
if (i == 0) return MarkAsReadWidget(narrow: widget.narrow);
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == 0) return const SizedBox(height: 36);

if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);

final data = model!.items[length - 1 - (i - 1)];
final data = model!.items[length - 1 - (i - 2)];
switch (data) {
case MessageListHistoryStartItem():
return const Center(
Expand All @@ -326,7 +334,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
case MessageListMessageItem():
return MessageItem(
key: ValueKey(data.message.id),
trailing: i == 1 ? const SizedBox(height: 8) : const SizedBox(height: 11),
trailingWhitespace: i == 1 ? 8 : 11,
item: data);
}
});
Expand Down Expand Up @@ -403,26 +411,23 @@ class MarkAsReadWidget extends StatelessWidget {
secondChild: SizedBox(width: double.infinity,
// Design referenced from:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=132-9684&mode=design&t=jJwHzloKJ0TMOG4M-0
child: ColoredBox(
// TODO(#368): this should pull from stream color
color: Colors.transparent,
child: Padding(
// vertical padding adjusted for tap target height (48px) of button
padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)),
child: FilledButton.icon(
style: FilledButton.styleFrom(
backgroundColor: _UnreadMarker.color,
minimumSize: const Size.fromHeight(38),
textStyle: const TextStyle(
fontFamily: 'Source Sans 3',
fontSize: 18,
height: (23 / 18),
).merge(weightVariableTextStyle(context)),
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)),
),
onPressed: () => _handlePress(context),
icon: const Icon(Icons.playlist_add_check),
label: Text(zulipLocalizations.markAsReadLabel(unreadCount)))))));
child: Padding(
// vertical padding adjusted for tap target height (48px) of button
padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)),
child: FilledButton.icon(
style: FilledButton.styleFrom(
backgroundColor: _UnreadMarker.color,
minimumSize: const Size.fromHeight(38),
textStyle: const TextStyle(
fontFamily: 'Source Sans 3',
fontSize: 18,
height: (23 / 18),
).merge(weightVariableTextStyle(context)),
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)),
),
onPressed: () => _handlePress(context),
icon: const Icon(Icons.playlist_add_check),
label: Text(zulipLocalizations.markAsReadLabel(unreadCount))))));
}
}

Expand All @@ -446,11 +451,11 @@ class MessageItem extends StatelessWidget {
const MessageItem({
super.key,
required this.item,
this.trailing,
this.trailingWhitespace,
});

final MessageListMessageItem item;
final Widget? trailing;
final double? trailingWhitespace;

@override
Widget build(BuildContext context) {
Expand All @@ -460,10 +465,12 @@ class MessageItem extends StatelessWidget {
header: RecipientHeader(message: message),
child: _UnreadMarker(
isRead: message.flags.contains(MessageFlag.read),
child: Column(children: [
MessageWithPossibleSender(item: item),
if (trailing != null && item.isLastInBlock) trailing!,
])));
child: ColoredBox(
color: Colors.white,
child: Column(children: [
MessageWithPossibleSender(item: item),
if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!),
]))));
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ 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(202);
check(itemCount(tester)).equals(203);

// Fling-scroll upward...
await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
Expand All @@ -94,7 +94,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(302);
check(itemCount(tester)).equals(303);
});

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