Add avatar list at the top right corner #3529

luka-nextcloud · 2021-11-02T07:54:44Z

Signed-off-by: Luka Trovic [email protected]
Change-Id: I39f567c67a727a23fdef3d98a147fa30a13b36a4

Resolves: #
Target version: master

Summary

TODO

...

Checklist

Code is properly formatted
All commits have Change-Id
I have run tests with make check
I have issued make run and manually verified that everything looks okay
Documentation (manuals or wiki) has been updated or is not required

juliusknorr · 2021-11-02T09:10:22Z

@luka-nextcloud Avatar display should work again after #3531 is merged

juliusknorr · 2021-11-02T09:12:12Z

loleaflet/html/loleaflet.html.m4

    <input id="selectbackground" type="file" accept="image/*" style="position: fixed; top: -100em">

    <div id="closebuttonwrapper">
+      <div class="userInfo">


This should rather be moved out of the closebuttonwrapper

juliusknorr · 2021-11-02T09:31:41Z

loleaflet/src/control/Control.UserList.js

+		$('#listActiveUser').append('<div class="user-item-wrapper" id="user-wrapper-' + viewId + '"><p class="user-item" id="user-' + viewId 
+		+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ userName +'</span></div>');
+
+		$('.userInfo').append('<p class="user-top ' + ownerClass + '" id="user-top-' + viewId + '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + ';"></p>');


Would be great if we could move the rendering of the header avatars to a separate method, ideally also just making use of plain javascript for the dom manipulation instead of jquery, so this doesn't have the external dependency on the library.

A few other points to consider:

The avatar might not be available

online/loleaflet/src/control/Control.UserList.js

Line 94 in 3ab824c

if (extraInfo !== undefined && extraInfo.avatar !== undefined) {

For uniquely identifying the element you could use a data-view-id="" attribute

Maintain the option to follow the current editor or a user

juliusknorr · 2021-11-02T09:33:02Z

loleaflet/css/toolbar.css

+}
+
+.guess {
+	opacity: 0.5;


I'd say we don't need a separation between the display of the current user or guest like that. Also opacity may look a bit odd when other avatars are shining through

juliusknorr · 2021-11-02T09:33:32Z

loleaflet/src/control/Control.UserList.js

 		var content = L.DomUtil.create('tr', 'useritem');
 		content.id = 'user-' + viewId;
 		$(document).on('click', '#' + content.id, this.onUseritemClicked.bind(this));
+		var ownerClass = userName == 'You' ? 'onwer' : 'guess';


@pedropintosilva Maybe something for you to decide if we should actually show the avatar of the current user view or just hide it.

luka-nextcloud · 2021-11-05T14:40:25Z

Here's a screenshot:

juliusknorr · 2021-11-05T15:12:10Z

loleaflet/src/control/Control.UserList.js


-	updateUserListCount: function() {
+	renderUserAvatars: function(viewId, userName, extraInfo, color) {
+		if (extraInfo !== undefined && extraInfo.avatar !== undefined) {


This check is duplicate with the one in https://github.com/CollaboraOnline/online/pull/3529/files#diff-41de9cb6f19860d72e4fe0ba9bae4682ea79c3312a3ca4ad3c1219cf43f0d8efR88

juliusknorr · 2021-11-05T15:13:24Z

loleaflet/src/control/Control.UserList.js

+	renderUserAvatars: function(viewId, userName, extraInfo, color) {
+		if (extraInfo !== undefined && extraInfo.avatar !== undefined) {
+			var avatarElement = '<div class="user-item-wrapper" id="user-wrapper-' + viewId + '"><p class="user-item" id="user-' + viewId 
+			+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ userName +'</span></div>';


Suggested change

+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ userName +'</span></div>';

+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ this.escapeHtml(userName) +'</span></div>';

juliusknorr · 2021-11-05T15:15:38Z

loleaflet/css/toolbar.css

+	align-items: center;
+}
+
+.user-item {


Please add somea dditional styling to make the spacing fit the existing popover entries and the font also match the default font-family: var(--loleaflet-font);

See screenshot for comparison:

loleaflet/html/loleaflet.html.m4

+    <div class="userInfo">
+      <div class="userInfo-content" id="listActiveUser">
+      </div>
+    </div>


loleaflet/src/control/Control.UserList.js

+			var avatarElement = '<div class="user-item-wrapper" id="user-wrapper-' + viewId + '"><p class="user-item" id="user-' + viewId 
+			+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ userName +'</span></div>';
+			if ($('#follow-editor').length == 0) {
+				$('#listActiveUser').append(avatarElement);


juliusknorr · 2021-11-11T13:41:43Z

@luka-nextcloud This would be the open points I can think of right now, most likely best tackled in the order mentioned 😉

Use the viewId as unique identifier as there might be users with the same
- https://github.com/CollaboraOnline/online/pull/3529/files#diff-216513472020c43e49874b42a0ed8e16b5332900a653a31e77b9adea39e5db91R131
Clicking the avatar should trigger the following of the user cursor (see current implementation)
The checkbox to follow the current editor should be working (see current implementation)
Do not show on mobile (there the current implementation works fine with just showing the number of users)
Hide the existing avatar list in the status bar if the new one is shown
Ideally do not remove all elements to rerender but check if an element is present already in the DOM and remove it if the view gets removed or add a new element if needed
In the end it would be good to replace jQuery usage with plain javascript methods
- https://youmightnotneedjquery.com/ is a good reference in that regard

pedropintosilva · 2021-11-16T16:08:50Z

@luka-nextcloud it seems there are some problems in the following lines:

browser/src/control/Control.UserList.js
  88:1  error  Expected indentation of 2 tabs but found 4 spaces  indent
  90:1  error  Expected indentation of 2 tabs but found 4 spaces  indent

Could you please fix that?

Note: make sure your text editor/IDE is using the proper JS linter with the appropriated configuration within online project:

browser/.eslintrc

luka-nextcloud · 2021-11-16T16:10:27Z

Sure, let me check!

pedropintosilva · 2021-11-18T13:10:02Z

This is still not passing the tests because

#userListSummary is now obstructing the view from other elements
- So when a user tries to click an icon in the toolbar it triggers instead that element
- That's why in the tests when trying to click #tb_editbar_item_linespacing it fails

Maybe removing the default margin top and bottom from the child (p#user-top-x) that is inside of #userListSummary would suffice in order to fix this

pedropintosilva · 2021-11-18T13:22:37Z

Probably the parent's position should also be tweaked after that, so maybe something like this :

+ #userListSummary p {
+   margin-top: 0;
+   margin-bottom: 0;
+ }

#userListHeader {
-  top: 5px;
+ top: 2px;
}

Signed-off-by: Luka Trovic <[email protected]> Change-Id: I058de8da8e4f3fbd0994c3e50efa7c1deb5be8ac

juliusknorr · 2021-11-19T12:08:17Z

@luka-nextcloud Lets continue over at #3677 as discussed 😉

pedropintosilva · 2021-11-19T18:14:00Z

this has been moved to #3677

juliusknorr reviewed Nov 2, 2021

View reviewed changes

luka-nextcloud force-pushed the update-avatar-list branch 2 times, most recently from 06232e8 to eefc4a8 Compare November 5, 2021 14:32

juliusknorr reviewed Nov 5, 2021

View reviewed changes

luka-nextcloud force-pushed the update-avatar-list branch from eefc4a8 to 11f5fbf Compare November 9, 2021 07:04

juliusknorr mentioned this pull request Nov 10, 2021

UI rework nextcloud/richdocuments#1820

Merged

luka-nextcloud force-pushed the update-avatar-list branch from 11f5fbf to ff5667a Compare November 11, 2021 13:16

luka-nextcloud force-pushed the update-avatar-list branch from ff5667a to b9c03f4 Compare November 16, 2021 08:49

luka-nextcloud force-pushed the update-avatar-list branch from b9c03f4 to 05638f9 Compare November 16, 2021 16:46

luka-nextcloud force-pushed the update-avatar-list branch from 05638f9 to cc1de4c Compare November 18, 2021 17:29

juliusknorr mentioned this pull request Nov 18, 2021

Move avatar list to the header #3677

Merged

5 tasks

add avatar list to the top right of the header

47e6b07

Signed-off-by: Luka Trovic <[email protected]> Change-Id: I058de8da8e4f3fbd0994c3e50efa7c1deb5be8ac

luka-nextcloud force-pushed the update-avatar-list branch from cc1de4c to 47e6b07 Compare November 18, 2021 18:33

pedropintosilva closed this Nov 19, 2021

	+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ userName +'</span></div>';
	+ '" style="background-image:url(\'' + extraInfo.avatar + '\');border-color:' + color + '"></p><span> '+ this.escapeHtml(userName) +'</span></div>';

Add avatar list at the top right corner #3529

Add avatar list at the top right corner #3529

Uh oh!

Conversation

luka-nextcloud commented Nov 2, 2021

Summary

TODO

Checklist

Uh oh!

juliusknorr commented Nov 2, 2021

Uh oh!

juliusknorr Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

juliusknorr Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

juliusknorr Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

juliusknorr Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

luka-nextcloud commented Nov 5, 2021

Uh oh!

juliusknorr Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

juliusknorr Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

juliusknorr Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

juliusknorr commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedropintosilva commented Nov 16, 2021

Uh oh!

luka-nextcloud commented Nov 16, 2021

Uh oh!

pedropintosilva commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedropintosilva commented Nov 18, 2021

Uh oh!

juliusknorr commented Nov 19, 2021

Uh oh!

pedropintosilva commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juliusknorr Nov 2, 2021 •

edited

Loading

juliusknorr commented Nov 11, 2021 •

edited

Loading

pedropintosilva commented Nov 18, 2021 •

edited

Loading