-
Notifications
You must be signed in to change notification settings - Fork 306
Redesigned DM recipient headers #426
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
Conversation
0e6a710
to
097032a
Compare
@gnprice rebased on main and ready for review! |
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.
Thanks @sirpengi! Generally this looks good — a few small comments.
} | ||
|
||
return GestureDetector( | ||
behavior: HitTestBehavior.translucent, |
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.
msglist: Remove HitTestBehavior.translucent on DmRecipientHeader
This seems unintended as the header should capture all
clicks within its bounds.
Capturing all clicks within its bounds is the reason this was translucent
. Here's that value's doc:
/// Translucent targets both receive events within their bounds and permit
/// targets visually behind them to also receive events.
translucent,
So with that value, the header does capture all taps within its bounds.
The default is deferToChild
. That one's doc says:
/// Targets that defer to their children receive events within their bounds
/// only if one of their children is hit by the hit test.
deferToChild,
So with that default, the header would not necessarily capture all taps within its bounds — it only does so if its child is hit.
This parameter is unnecessary now, though, so it's good to remove it. The reason it's unnecessary is that the child fills the whole box.
Looking at the history with git log -L
(git log -L 668,693:lib/widgets/message_list.dart
as of the tip of your branch, to cover this whole return
statement), I believe the key change was 412233c. Before that, and when this line was added in f7feabf as part of making the DM recipient header touchable at all, the child was an Align
and so it only occupied the left-hand portion of the parent. That made HitTestBehavior.translucent
necessary in order for touching everywhere in the header to work.
assets/l10n/app_en.arb
Outdated
}, | ||
"messageListGroupYouAndOthers": "You and {others}", | ||
"@messageListGroupYouAndOthers": { | ||
"description": "Message list sticky header for a dm group with others.", |
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.
"description": "Message list sticky header for a dm group with others.", | |
"description": "Message list recipient header for a DM group with others.", |
"DM" is an initialism, so in normal English text (as opposed to identifiers in code) it's all caps.
Also although these headers are sticky, I think "recipient header" works better as a name for them because it points more to what they're about.
assets/l10n/app_en.arb
Outdated
@@ -194,6 +194,21 @@ | |||
"filename": {"type": "String", "example": "file.txt"} | |||
} | |||
}, | |||
"messageListGroupUnknownUser": "(unknown user)", |
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.
"messageListGroupUnknownUser": "(unknown user)", | |
"unknownUserName": "(unknown user)", |
I think it makes sense to use the same form for this pretty consistently across the app — seems helpful for reducing confusion about whether it literally means an unknown user or the name some actual user has picked. So we can make the identifier more general.
assets/l10n/app_en.arb
Outdated
@@ -194,6 +194,21 @@ | |||
"filename": {"type": "String", "example": "file.txt"} | |||
} | |||
}, | |||
"messageListGroupUnknownUser": "(unknown user)", | |||
"@messageListGroupUnknownUser": { | |||
"description": "Name placeholder to use for user when we don't know their name." |
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.
"description": "Name placeholder to use for user when we don't know their name." | |
"description": "Name placeholder to use for a user when we don't know their name." |
The headline-ese "user" without an article sounds like it might mean "the user", i.e. the self-user.
097032a
to
44eab3e
Compare
@gnprice ready for review again! |
Contents of the header fill the whole box so this parameter is unnecessary.
Thanks for the revision! All looks good — merging. |
44eab3e
to
d6afeee
Compare
This is built on top of #416.
This PR updates the DM recipient headers to the new style. Text style and spacing was taken from the same sources as #416:
But given there were no mockups that included a DM recipient header the style used here also looked to how web renders these headers (in particular the color to use for the background, and icon to use for display).
Here showing behavior when the system requests a larger font:
