Skip to content
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.

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

msglist: Have content pad bottom inset when the compose box isn't shown #379

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

chrisbobbe
Copy link
Collaborator

@chrisbobbe chrisbobbe commented Nov 11, 2023

This takes advantage of some of StickyHeaderListView's default behavior (inherited from ListView). From the doc on ListView:

https://api.flutter.dev/flutter/widgets/ListView-class.html

By default, ListView will automatically pad the list's scrollable
extremities to avoid partial obstructions indicated by
MediaQuery's padding. To avoid this behavior, override with a zero
padding property.

The code changes here are just to arrange for that default behavior to help us out: we let the real height of the bottom inset propagate down to the StickyHeaderListView, with the MediaQuery mechanism.

Fixes: #263

@chrisbobbe
Copy link
Collaborator Author

Before After
image image

@chrisbobbe
Copy link
Collaborator Author

Also, the layout is unchanged when the compose box is shown:

Before After
image image

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for this fix! This had been bugging me occasionally before the mark-as-read button, too.

Does #263 correspond to what this fixes?

One small code comment below.

// The compose box, when present, pads the bottom inset.
// TODO this copies the details of when the compose box is shown;
// if those details get complicated, refactor to avoid copying.
removeBottom: widget.narrow != const AllMessagesNarrow(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
removeBottom: widget.narrow != const AllMessagesNarrow(),
removeBottom: widget.narrow is! AllMessagesNarrow,

Because we override ==, the two forms are actually exactly equivalent. But the is form is what appears in the ComposeBox logic, and I think is also the right conceptual framing of it. (For example if we somehow had two variants of AllMessagesNarrow — perhaps a boolean for whether to exclude muted? — then they'd still presumably both skip the compose box.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Collaborator Author

@chrisbobbe chrisbobbe Nov 15, 2023

Choose a reason for hiding this comment

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

(For example if we somehow had two variants of AllMessagesNarrow — perhaps a boolean for whether to exclude muted? — then they'd still presumably both skip the compose box.)

Yeah. Incidentally, I've become a little wary that AllMessagesNarrow might not always mean what we think it means. Here's its dartdoc:

/// The narrow called "All messages" in the UI.
///
/// This does not literally mean all messages, or even all messages
/// that the user has access to: in particular it excludes muted streams
/// and topics.

And yet:

  1. Doesn't the "All messages" message list include even muted messages?
  2. When we .apiEncode() this narrow and send it to the server for mark-all-as-read, it's taken to mean all messages, including muted ones.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Doesn't the "All messages" message list include even muted messages?

It does, but that's because we haven't implemented muting yet. When we implement #346, one piece of it is to have the muting control

which messages are shown in stream narrows and the all-messages narrow.

2. When we .apiEncode() this narrow and send it to the server for mark-all-as-read, it's taken to mean all messages, including muted ones.

Hmm true. And that's intended behavior: it's the same as the specialized endpoint https://zulip.com/api/mark-all-as-read does, and the same as the web UI does.

So possibly the better way to conceptualize this narrow (vs. what the quoted doc comment says) is that the narrow does include muted messages, but the message list only shows a subset of the messages in the narrow, namely those that aren't muted.

I'll think more about that when I take up #346.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does, but that's because we haven't implemented muting yet.

Huh, TIL (or today I reminded myself) that zulip-mobile's "All messages" view also seems to filter out messages in muted conversations. 😅 (I almost never use that view myself.)

@gnprice gnprice added the a-msglist The message-list screen, except what's label:a-content label Nov 13, 2023
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Nov 15, 2023

Does #263 correspond to what this fixes?

…Er, now it does! 😂 Thanks for making that connection! Revision pushed.

Updated to get the scroll-to-bottom button out of the inset, and to add TODOs about how we'd do things differently if we do a bottom nav, which would pad the bottom inset. Quoting myself from #263:

This will be naturally solved if we put some surface down there that pads the bottom inset, with the message list abutting that surface in the y-direction. That surface is likely to be the bottom nav bar in Vlad's design, I think.

Comment on lines +255 to +256
child: SafeArea(
child: ScrollToBottomButton(
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, I see.

@gnprice
Copy link
Member

gnprice commented Nov 15, 2023

Great, thanks! Looks good; merging.

This takes advantage of some of StickyHeaderListView's default
behavior (inherited from ListView). From the doc on ListView:

https://api.flutter.dev/flutter/widgets/ListView-class.html
> By default, ListView will automatically pad the list's scrollable
> extremities to avoid partial obstructions indicated by
> MediaQuery's padding. To avoid this behavior, override with a zero
> padding property.

The code changes here are just to arrange for that default behavior
to help us out: we let the real height of the bottom inset propagate
down to the StickyHeaderListView, with the MediaQuery mechanism.
@gnprice gnprice merged commit 627515b into zulip:main Nov 15, 2023
@chrisbobbe chrisbobbe deleted the pr-msglist-pad-bottom-inset branch November 16, 2023 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-msglist The message-list screen, except what's label:a-content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

msglist: In "All messages", content and the scroll-to-bottom button render in iOS bottom inset
2 participants