-
Notifications
You must be signed in to change notification settings - Fork 219
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
41 show user avatar #155
41 show user avatar #155
Conversation
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 @aanelson! Looking forward to getting this in. A few comments below.
One overall comment that applies to several spots below is that in the Zulip project, we make a practice of keeping a clean commit history: each Git commit does a single thing and does it in a self-contained way. More discussion here:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html
So in particular that means that
- we'll want the edits to the
avatar
computation and theactions
property in the latter two commits to get squashed into the same commit that makes the other main changes; - we'll want other, unrelated, changes to get separated out into their own commits.
Relatedly, we have a preferred style for commit messages that helps us scan the history for release management and debugging. For examples, see git log
in the repo.
.vscode/settings.json
Outdated
"editor.formatOnSave": false, | ||
"[dart]": { | ||
"editor.formatOnSave": false |
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.
This is helpful, thanks!
It should go in its own commit, as I mention above. A good summary line for that commit would be
vscode: Disable format-on-save
pubspec.lock
Outdated
sha256: "6501fbd55da300384b768785b83e5ce66991266cec21af89ab9ae7f5ce1c4cbb" | ||
sha256: "1803e76e6653768d64ed8ff2e1e67bea3ad4b923eb5c56a295c3e634bad5960e" | ||
url: "https://pub.dev" | ||
source: hosted | ||
version: "0.12.15" | ||
version: "0.12.16" |
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.
Yeah, sorry for the noise in this file; I believe #151 will cover this update. (And #85 tracks the fact that these happen spontaneously in the first place; #15 is how we plan to fix it.)
Like other unrelated changes, this should go in its own commit. I'll probably leave that commit out when merging (in favor of letting #151 cover it), but having it in the branch should be helpful for you because it means the file won't get the same spontaneous edit again.
lib/widgets/lightbox.dart
Outdated
return FadeTransition( | ||
opacity: animation.drive(CurveTween(curve: Curves.easeIn)), | ||
child: child); | ||
opacity: animation.drive(CurveTween(curve: Curves.easeIn)), | ||
child: child); |
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.
We use consistent two-space indentation, so we won't want this change. Similarly the return _LightboxPage(
change above.
Since you're still occasionally getting unintended auto-formatting, if it's helpful for you to include a separate commit that just applies the auto-formatting to this file, then I'd be fine with that as a workaround for this PR; I'll just leave that commit out when merging.
I'd definitely be interested in debugging that persistent VS Code auto-formatting you're seeing, though. (Both Chris and I use Android Studio in our Flutter codebase, so it's not something we've run into directly.)
lib/widgets/lightbox.dart
Outdated
final avatar = avatarUrl != null | ||
? Padding( | ||
padding: const EdgeInsets.all(8.0), | ||
child: ClipOval( |
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.
So this causes the avatar to get clipped to a circle.
That's a reasonable design choice in general, but in Zulip we've made a design choice that avatars are squares with only slightly-rounded corners, not circles. The general rationale for that is discussed in this thread; but for any given context like this one, I think the strongest reason is just that it's important to be consistent with users' expectations for how Zulip shows their avatars, which are shaped by how they see Zulip show avatars in other contexts.
Let's therefore clip this with the same shape we use in the other place we currently show avatars, for the sender in the message list.
Mmm, yeah, that makes sense. This is a good reminder that we should go ahead and take care of that; I've just bumped it up our priority list. |
162263d
to
968aca8
Compare
968aca8
to
04832ef
Compare
@gnprice Updated. I don't know of a way to keep the comments in the correct location when updating the commits so that will be broke, but I have addressed the issues. |
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 @aanelson for the revision! I appreciate the cleanly separated commits.
Comments below on the code, and some observations from playing with this version on a device.
width: 35, | ||
height: 35, | ||
decoration: const BoxDecoration( | ||
borderRadius: BorderRadius.all(Radius.circular(4))), |
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.
width: 35, | ||
height: 35, |
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.
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.
height: 35, | ||
decoration: const BoxDecoration( | ||
borderRadius: BorderRadius.all(Radius.circular(4))), | ||
child: RealmContentNetworkImage(avatarUrl), |
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.
In the other place where we show avatars, we use filterQuality: FilterQuality.medium
. Is there a specific reason to use a different filterQuality
here?
|
||
final avatar = avatarUrl != null | ||
? Padding( | ||
padding: const EdgeInsets.fromLTRB(8.0,4,0,0), |
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.
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:
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.)
{ | ||
"editor.tabSize": 2, | ||
"editor.formatOnSave": false, | ||
"dart.enableSdkFormatter": false, |
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.
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, |
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.
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?
Got basic avatar functionality. Also checked in something to disable auto formatting with vscode. It still occasionally doesn't work and formats the whole file.
Was writing test to verify the close behavior, but was blocked by #30.
Used ClipOval instead of CircleAvatar as that takes an ImageProvider instead of a widget and there is currently nothing setup to generate it with the headers.