Show read receipts #1706
Show read receipts #1706
Conversation
This fixes the two inconsistencies flagged in discussion: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/bottom.20sheet.20.22Cancel.22.2F.22Close.22.20button/near/2216116 > I think it's reasonable to have both labels, but I think we should > choose them differently than now: > > - "Cancel" when the sheet is about doing an action: [etc.] > > - "Close" when the sheet just presents information or nav options: > [etc.]
|
We should probably use the same icon as in the web app here: 
|
| color: designVariables.title, | ||
| ).merge(weightVariableTextStyle(context, wght: 600))), | ||
| if (status == FetchStatus.success && receiptCount > 0) | ||
| StyledText( |
There was a problem hiding this comment.
Very interesting, I didn't expect to be able to do this! Is this a solution to #1285?
There was a problem hiding this comment.
Looking at the issue, I think this could be a solution for it!
There was a problem hiding this comment.
Hmm interesting.
This wouldn't be a satisfactory solution to all of #1285, because this appears to fire up a whole XML parser for each one of these widgets, which is going to be expensive. That's acceptable in this context, where there's only one of these at a time (and it isn't a very common part of the UI anyway). It wouldn't be OK for something that could appear in, say, message content, where you might scroll past a lot of them.
…lainText This way, it can be used for purposes other than being a header, such as in the next commits, when read receipts fail to load, we use this widget in the middle of bottom sheet to give feedback to the user. This also adds a TextAlign property for controlling the alignment.
sm-sayedi
force-pushed
the
667-read-receipts
branch
from
7c6c52c to
d05ac9d
Compare
| } | ||
| } | ||
|
|
||
| class ViewReadReceiptsButton extends MessageActionSheetMenuItemButton { |
There was a problem hiding this comment.
An intermediate commit has:
@override void onPressed() {
// TODO: open read receipts sheet
}So it shows the button but the button doesn't do anything. That's a buggy state, so let's avoid it.
That commit is quite small, so we can just squash it into the later commit that gives this button something to do.
| /// A plain text widget for a bottom sheet with a multiline UI string. | ||
| /// | ||
| /// Comes with built-in 16px horizontal padding. | ||
| /// | ||
| /// Figma: | ||
| /// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3481-26993&m=dev | ||
| class BottomSheetPlainText extends StatelessWidget { |
There was a problem hiding this comment.
This name is very general. It's hard for me to tell what sort of situations this is meant for, or what it does.
If I want plain text, that's what Text does, right? It's not clear how the bottom sheet interacts with that.
| child: status == FetchStatus.loading | ||
| ? CircularProgressIndicator() | ||
| : status == FetchStatus.error | ||
| ? BottomSheetPlainText( |
There was a problem hiding this comment.
This chain of ?: would be best as a switch (status). That way it's clear we handle all the possible values.
| final localizations = ZulipLocalizations.of(context); | ||
| final designVariables = DesignVariables.of(context); | ||
|
|
||
| return Center( |
There was a problem hiding this comment.
Does this Center end up doing anything in the three cases other than CircularProgressIndicator? I suspect it doesn't.
| // TODO: deduplicate the code with [ViewReactionsUserItem] | ||
| @visibleForTesting | ||
| class ReadReceiptsUserItem extends StatelessWidget { | ||
| const ReadReceiptsUserItem(this.pageContext, { |
There was a problem hiding this comment.
nit: widget parameters should always be named (with very rare exceptions like Text)
| padding: EdgeInsets.symmetric(vertical: 8), | ||
| itemCount: userIds.length, | ||
| itemBuilder: (context, index) => | ||
| ReadReceiptsUserItem(context, userId: userIds[index])))); |
There was a problem hiding this comment.
The child widget here calls this argument pageContext, but the context we're passing looks like just its own parent's context. If this context works, then I expect the child's own context would work equally well.
(This is also an example of why it's good for widget parameters to be named — if this said pageContext: context, then the contrast would jump out more.)
| /// https://zulip.com/api/get-read-receipts | ||
| Future<GetReadReceiptsResult> getReadReceipts(ApiConnection connection, { | ||
| required int messageId, | ||
| }) { | ||
| return connection.get('getReadReceipts', GetReadReceiptsResult.fromJson, | ||
| 'messages/$messageId/read_receipts', null); |
There was a problem hiding this comment.
This should go in lib/api/ :slightly_smiling_face:
| color: designVariables.title, | ||
| ).merge(weightVariableTextStyle(context, wght: 600))), | ||
| if (status == FetchStatus.success && receiptCount > 0) | ||
| StyledText( |
There was a problem hiding this comment.
Hmm interesting.
This wouldn't be a satisfactory solution to all of #1285, because this appears to fire up a whole XML parser for each one of these widgets, which is going to be expensive. That's acceptable in this context, where there's only one of these at a time (and it isn't a very common part of the UI anyway). It wouldn't be OK for something that could appear in, say, message content, where you might scroll past a lot of them.
Support for viewing read receipts of a message.
Rebased on top of #1700 to borrow some code from. Main commits start from 17fa9ff "icons: Add
fact_checkicon, from Figma".Figma link: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=11367-20647&t=bpmAb3Fr9aJdMIHw-0
TODO:
Screenshots
Screen recording
Read.receipts.-.Screen.recording.mov
Fixes: #667