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.

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

41 show user avatar #155

Closed
wants to merge 4 commits into from
Closed

41 show user avatar #155

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
cef392a
vscode: updated project settings.
aanelson Jun 8, 2023
f61b0cc
dev: updated podfile.lock
aanelson Jun 8, 2023
30f7abe
style: Updated format to match dart/flutter style
aanelson Jun 8, 2023
04832ef
lightbox: added avatar image to top app bar
aanelson Jun 8, 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
9 changes: 9 additions & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"editor.tabSize": 2,
"editor.formatOnSave": false,
"dart.enableSdkFormatter": false,
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I just spotted that your new revision added this line. Did this turn out to be the solution to this problem you mentioned above?

[…] checked in something to disable auto formatting with vscode. It still occasionally doesn't work and formats the whole file.

If so, that's good to hear!

"[dart]": {
"editor.formatOnSave": false,
"editor.tabSize": 2,
Comment on lines +5 to +7
Copy link
Member

Choose a reason for hiding this comment

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

These look like they're restating some of the settings that already appear globally above. Is there an additional effect that's had by restating them for the specific language?

}
}
2 changes: 1 addition & 1 deletion ios/Podfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,4 @@ SPEC CHECKSUMS:

PODFILE CHECKSUM: 985e5b058f26709dc81f9ae74ea2b2775bdbcefe

COCOAPODS: 1.11.3
COCOAPODS: 1.12.1
gnprice marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 23 additions & 3 deletions lib/widgets/lightbox.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,32 @@ class _LightboxPageState extends State<_LightboxPage> {
.add_Hms()
.format(DateTime.fromMillisecondsSinceEpoch(widget.message.timestamp * 1000));

final avatarUrl = widget.message.avatarUrl == null // TODO get from user data
? null // TODO handle computing gravatars
: resolveUrl(widget.message.avatarUrl!, PerAccountStoreWidget.of(context).account,
);

final avatar = avatarUrl != null
? Padding(
padding: const EdgeInsets.fromLTRB(8.0,4,0,0),
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this is ending up with a very thin strip of padding between the bottom of the image and the bottom of the app bar, which comes out looking kind of odd:
lightbox-avatar-155-padding

I think putting at least 4px of padding at the bottom too makes it look significantly better.

Is there a particular reason for putting 8px at the left but 4px at the top? I think the top-left and bottom-left corners may look better if the top, left, and bottom padding are all equal. (The right is less visible because there isn't a visual boundary there.)

child: Container(
clipBehavior: Clip.antiAlias,
width: 35,
height: 35,
Comment on lines +153 to +154
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like this is actually ending up 35px tall or wide; it's bigger than that. I think what's happening is that the AppBar hands down tight size constraints to its leading child, and so those constraints prevail.

The size it's actually getting is fine; the sender avatars in the message list are 35px, but there's no reason these need to be the same size. We should leave out these lines given that they aren't actually having an effect.

decoration: const BoxDecoration(
borderRadius: BorderRadius.all(Radius.circular(4))),
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, when trying out this PR, something seems off about the rounding on the corners. Here's from an iPhone 13 Pro:
lightbox-avatar-155

It looks like the clipped-off parts of the corners are taller than they are wide, at each of the four corners.

child: RealmContentNetworkImage(avatarUrl),
Copy link
Member

Choose a reason for hiding this comment

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

In the other place where we show avatars, we use filterQuality: FilterQuality.medium. Is there a specific reason to use a different filterQuality here?

),
)
: const SizedBox.shrink();


appBar = AppBar(
centerTitle: false,
foregroundColor: appBarForegroundColor,
backgroundColor: appBarBackgroundColor,

// TODO(#41): Show message author's avatar
leading: avatar,
actions: const [CloseButton()],
title: RichText(
text: TextSpan(children: [
TextSpan(
Expand Down Expand Up @@ -205,7 +225,7 @@ class _LightboxPageState extends State<_LightboxPage> {
Route getLightboxRoute({
required BuildContext context,
required Message message,
required String src
required String src,
}) {
return AccountPageRouteBuilder(
context: context,
Expand Down
3 changes: 2 additions & 1 deletion lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ class MessageWithSender extends StatelessWidget {
borderRadius: BorderRadius.all(Radius.circular(4))),
width: 35,
height: 35,
child: avatar)),
child: avatar,
)),
Expanded(
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
Expand Down