adjust avatar and its line height to comment message style

[blizzz]

@jancborchardt @dan0xii there is still something odd with line breaks when the message is rendered. It's seems right in the edit mode (content-editable=true), but not in the plain one.

Before:

Now:

[skjnldsv]

I'm still really not convinced by how we display our avatars in comments. :/

[skjnldsv]

I would prefer something more inline like this, what do you think @nextcloud/designers ?

[dan0xii]

@blizzz, I think you meant to cc @danxuliu.

[codecov]

Codecov Report

Merging #7514 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7514      +/-   ##
============================================
+ Coverage     51.16%   51.17%   +<.01%     
  Complexity    24865    24865              
============================================
  Files          1601     1601              
  Lines         94683    94688       +5     
  Branches       1368     1369       +1     
============================================
+ Hits          48446    48452       +6     
+ Misses        46237    46236       -1

Impacted Files	Coverage Δ	Complexity Δ
apps/comments/js/commentstabview.js	84.4% <100%> (+0.24%)	0 <0> (ø)	⬇️
apps/files_trashbin/lib/Trashbin.php	72.53% <0%> (+0.24%)	136% <0%> (ø)	⬇️

[danxuliu]

@skjnldsv

I would prefer something more inline

Mmm, but if you do that then whether the display name is perfectly legible or not would depend on the colours of the avatar...

In any case, regarding this pull request, besides the avatar size let's not forget about the other issues ;) In fact I would solve them first and then fix the size, as changing the size first may mask without really solving some of them.

But first of all it would be better to decide if we are keeping the avatars shown like they are now or not to avoid working in vain ;-)

[MorrisJobke]

I would drop the avatars from the inline text. No need to clutter the text like this.

[skjnldsv]

@danxuliu there is other ways to ensure that white text is readable over an undefined background, we do it in multiple places like the login page for example. Visually speaking, how do you like my example?

[jancborchardt]

@skjnldsv overlaying it like this will impair readability in any case. Also, we don't do it like this anywhere else, nor is there really a popular platform which does this.

I would say this is great, as long as the vertical alignment and margin to the username is proper and the same.
The avatars help a lot with recognizing people even when small. :)

[danxuliu]

I would drop the avatars from the inline text. No need to clutter the text like this.

I would keep them, but smaller like in this pull request and closer to the display name; sort of a replacement for the @ symbol.

there is other ways to ensure that white text is readable over an undefined background, we do it in multiple places like the login page for example

Do you mean using text-shadow? Personally I do not think that it is legible enough (OK, I have used the worst case in the screenshot below, but still):

The best way to ensure that a text is legible regardless of the background would be with a text outline instead of a text shadow, but it seems that it was removed from CSS3 :-(

Visually speaking, how do you like my example?

I still prefer the avatar to the side instead of below the display name, sorry :-)

[skjnldsv]

@danxuliu indeed! :)
Using a filter blend could work, too ;)

I don't like having the avatar in front of it, I feel like it's two separate elements, I like the idea of having both of them merged together. I won't vote then! :)

[jancborchardt]

@skjnldsv as I mentioned, we can move them closer together. But merging would be absolutely non-standard and very difficult to read, especially for people with visual impairments. It also won't work with the avatar placeholders as they have an actual letter in them which further hampers readability.

[pixelipo]

I'm strongly agreeing with @MorrisJobke on this topic. Avatars are great when you need to identify author, but inline, they're nothing but hieroglyphs. Since we are not living in ancient Egypt or Mesopotamia, we should be able to read text using latin alphabet only.

In fact, has anyone bothered to check how this looks with Mandarin, Arabic etc.?

[skjnldsv]

Since we are not living in ancient Egypt or Mesopotamia

How do you know? :O
I have the rosetta stone in my living room!

[jancborchardt]

Folks, there are compelling reasons for including the avatars. Names with the avatars directly put a face to the person and are like an easy-to-recognize "icon" of them. Like when scrolling through a conversation on Github or Signal or any messenger, you don't really read the names.

Furthermore, it makes it evidently clear someone is mentioning a person. Otherwise it's just bolded text.

Also keep in mind that the example above is a bit nonstandard since you would not mention the same person 3 times in the same comment really.

With the small avatar it's absolutely fine. I will look into proper alignment later.

[blizzz]

@blizzz, I think you meant to cc @danxuliu.

Oops, yes, sorry 🙇

[danxuliu]

@skjnldsv

Using a filter blend could work, too ;)

Interesting technique :-) Maybe someday I will have time to get up to date with the new CSS features... :-P

I don't like having the avatar in front of it, I feel like it's two separate elements

Quoting myself:

I would keep them, but smaller like in this pull request and closer to the display name; sort of a replacement for the @ symbol.

and @jancborchardt:

we can move them closer together

Gestalt law of proximity FTW! :-P

@pixelipo

In fact, has anyone bothered to check how this looks with Mandarin, Arabic etc.?

Right now, if Nextcloud was used in a right-to-left language like Arabic the look of the avatars will probably be the lesser of the problems :-P

Jokes aside, as far as I know characters in those languages are still characters. I mean, even if they can be very complex like Chinese characters they are still monochromatic, so as long as the display name is shown in bold and the avatar is closer to it than to the previous character it would be the same as in Latin characters, wouldn't it? Or what I am missing?

[blizzz]

I am also for keeping avatars with the display names, and I also agree with moving them closer together. Fancy tricks are fancy, but I would not overlay them alone for accessibility, even if you can do some nice stuff with filters and what not. → Seconding @jancborchardt

With the small avatar it's absolutely fine. I will look into proper alignment later.

Yay! Any contributions to this dark art are highly appreciated 😎

[pixelipo]

@jancborchardt since you're mentioning Github, as is obvious in this comment - your username is still very visible, even though it doesn't include an avatar.
Facebook, Github, Slack (Jira, Bitbucket) - they all show avatars for comment authors, but none shows avatars for mentions... In fact, I don't know of any applications that adds avatars to inline user mentions.

I don't have a problem with readability here:

But add avatars to those mentions and you get a total mess (5 extra icons to mess with reading flow).

[danxuliu]

as long as the display name is shown in bold and the avatar is closer to it than to the previous character it would be the same as in Latin characters, wouldn't it? Or what I am missing?

OK, I have realized that what I was missing is that the spacing used in logogram-based writing systems is not necessarily the same as in alphabet and syllabary-based languages (as the characters are logograms they could be written with the same spacing between each of them instead of grouped in words like in alphabets and syllabaries).

Anyway, I think that is not a reason to drop avatars; I am sure that there are a lot of other details in logogram-based languages to take into account if an alternative to image avatars was needed due to those writing systems.

[jancborchardt]

@pixelipo the problem with readability there is that among the lowercased, similar length usernames, you actually have to read them to distinguish. Avatars help to identify people way quicker, and they don't disturb text flow. People also use emojis quite a lot, which are similar in size. 😉

[skjnldsv]

you actually have to read them to distinguish

The only username you can't mix up with another one is mine! 😆

[pixelipo]

OK, one last try proving my point and I'm out ☠️

1. emojis are icons, specially designed for font size.
2. avatars are usually not icons - they are usually photos
3. if it made sense to have tiny avatars, they why do we have 32*32px avatars?
4. if it made sense to have tiny avatars, then the avatar in the header would also be 1616 - same size as the icons it is placed next to (or perhaps 2020 - size of the app icons); now it's sticking out of the place - and for a good reason
5. if no avatar is set, a default one is created. That avatar contains an icon-sized letter. What would be the size of that letter if we start displaying tiny avatars?
6. default avatars end up looking like this: https://upload.wikimedia.org/wikipedia/commons/1/1d/Fraktur.png
7. you have to read a username to distinguish? Of course you have to read; in fact, that's what users usually do when faced with a comment - the read them.
8. comment authors have avatars, because user sometimes doesn't want to read all comment - but only one, written by a particular author. But once user is dedicated to actually reading a comment, then the whole comment is read, including usernames; I see no situation where user doesn't want to read the comment, but want to see who was mentioned in it.
9. User who actually starts reading, doesn't want (and shouldn't be) distracted from that one action. Emojis are fun and perform a particular function (assign author's emotion to written text) that can be useful to understand context of the message. Avatars don't help with transmitting intended information from author to reader.
10. Emojis are essentially fonts - they belong in text. Avatars are not.
11. Avatars add a whole lot of DOM that could spell disaster for accessibility (screen readers).
12. In large companies where setting a personal avatar is not mandatory, they could actually hinder readability of a comment (is that "J" for Jan or "Johnny"?)

That's all from me on this topic 👋

[skjnldsv]

Point proven @pixelipo :)

[rullzer]

So what is the verdict here? Beta 4 is coming. Last chance to get this is. If there is discussion left I suggest to move it to 14 when we find a suitable solution.

[blizzz]

@pixelipo arguments convince me. Do we all agree on? I'd adapt the PR then.

[MorrisJobke]

@pixelipo arguments convince me. Do we all agree on? I'd adapt the PR then.

I would say so. 👍

Let's move this to 14.

[rullzer]

Yes makes a lot of sense.!

[jancborchardt]

While I disagree, through points stated above, I don't want to piss awesome folks off – so let's simply go without the avatars inside of comment text. :)

(We can revisit it in 2-5 years when others might start doing it. ;)

[nickvergessen]

Jan asked me to use 21px for avatars in activities.
Maybe we can use the same here.

[nickvergessen]

Ref #2012

[blizzz]

obsolete due to #7514 (comment) ff

[jancborchardt]

And btw for the record, one product which does this is https://about.riot.im (usable IRC chat ;)

cc @skjnldsv @pixelipo just for input :)

[blizzz]

usable

Cannot test, requires side loaded, 3rd party service from an ads enterprise. Blocked here. #poweruser

[jancborchardt]

@blizzz feel free to run it yourself: https://github.com/vector-im/riot-web

Also, the screenshots I posted above show everything you need to see.

[MorrisJobke]

@nextcloud/designers We need a decision on this. What is the current status? Show it or not? Keeping a PR open for that long doesn't help anybody. 😢

[danxuliu]

@MorrisJobke The idea is to remove the avatars from mentions and unify their look with the ones used in Talk, so I think that we can close this pull request, as it will be overridden sooner or later (hopefully sooner than later ;-) ).