Fix comment mentions in activities

[nickvergessen]

Before

After

@blizzz @MorrisJobke @jancborchardt

[codecov-io]

Current coverage is 57.49% (diff: 0.00%)

Merging #1926 into master will increase coverage by 0.26%

@@             master      #1926   diff @@
==========================================
  Files          1079       1079          
  Lines         61585      62239   +654   
  Methods        6886       6983    +97   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35242      35784   +542   
- Misses        26343      26455   +112   
  Partials          0          0          

Diff Coverage	File Path
0%	apps/comments/lib/Activity/Extension.php

Powered by Codecov. Last update f2cae3c...2864826

[MorrisJobke]

@nextcloud/designers Do we really want to have avatars inside the text? I think this is really awkward - also because the icon is a lot higher than the line height and destroys the view of a text.

[nickvergessen]

Well this just copies the comments view, so I guess.
And yes @jancborchardt said usernames shall always have avatars next to them

[LukasReschke]

@nextcloud/designers Do we really want to have avatars inside the text? I think this is really awkward - also because the icon is a lot higher than the line height and destroys the view of a text.

I think it looks ok. Especially since you usually don't mention 40 people on a comment :)

[eppfel]

I agree, that that inline avatars could be improved and I don't like dogmas like "usernames shall always have avatars", but it looks ok to me for now.

[blizzz]

@nickvergessen oops, you stepped into the same trap i did…

• BUG: The first mention has id user_1 and the second user_11, of which only the user_1 part was replaced.

[blizzz]

Aside of this, avatars in activities are ok… they are already present with shares, too. With a lot of users it can look very colorful, however in production the benefit might be bigger. Perhaps it's also an experience that we need to make in usage and adjust if necessary?

[jancborchardt]

also because the icon is a lot higher than the line height and destroys the view of a text

@MorrisJobke That’s a problem we need to fix then (separately).

I don't like dogmas like "usernames shall always have avatars"

@eppfel Why not though? Same for filenames, they should always have the preview or filetype icon next to them. It’s not only consistent but also very easy to recall, and makes the whole interface friendlier.

Anyway – as the others said it looks good! 👍

[MorrisJobke]

BUG: The first mention has id user_1 and the second user_11, of which only the user_1 part was replaced.

This is still broken - beside that it works nicely :)

[nickvergessen]

Updated

[MorrisJobke]

Tested and works 👍

I already retriggered the build

[eppfel]

@eppfel Why not though? Same for filenames, they should always have the preview or filetype icon next to them. It’s not only consistent but also very easy to recall, and makes the whole interface friendlier.

@jancborchardt I don't have a particular case for nextcloud, I just don't like "always". I have the impressions it is easier to read through the list of github mentions, than the list of nextcloud commenters:

And if used on github, inline avatars have the same line height as the text.

[MorrisJobke]

And if used on github, inline avatars have the same line height as the text.

We should move that discussion out of this PR -> this is ready to be merged

[jancborchardt]

@eppfel want to open a new issue? One consideration for this context could be to show the name without placeholder when there’s no avatar. And show only the avatar of everyone when there’s more than 3 people.