Avatars in share dialog fixes #14582
Conversation
| $('#avatar-group-' + escapeHTML(shareWith)).imageplaceholder(shareWith + ' group', shareWith); | ||
| } else if (shareType === OC.Share.SHARE_TYPE_REMOTE) { | ||
| var tmpShareWith = escapeHTML(shareWith).replace(/[@\.\/]/g,"_"); | ||
| $('#avatar-remote-' + tmpShareWith).imageplaceholder(shareWith); |
There was a problem hiding this comment.
The code looks ok, but I'm a bit worried that this is getting big on code duplication. But also I don't have any good idea yet how to make this look better.
There was a problem hiding this comment.
Would be cool if we could find a way to not need to distinguish the ids between "#avatar-{user,group,remote}" but only have the same div format for all. This might be possible by using data attributes, for example "data-sharetype='user'" and "data-sharewith='someuser'".
There was a problem hiding this comment.
Ah that could work indeed. Would require some rewriting of the avatar code. Or maybe a new function since extra logic is required. But should be doable.
There was a problem hiding this comment.
As you like: optional.
I'm not sure it's worth spending that time, especially if at some point we want to rewrite this as a popup dialog anyway (and with using handlebars)
There was a problem hiding this comment.
Yeah better then to keep it a bit duplicated for now. And make it all nice and fancy once the popup dialog is available.
|
This PR will clash with: #14585 @LukasReschke did use a custom attribute there, a bit like we discussed above. |
|
Yes – don't do what you do right now 🙈 |
|
Ah that looks better indeed. I'll have a look. |
| $('.avatar[data-user="' + escapeHTML(shareWith) + '"]').avatar(escapeHTML(shareWith), 32); | ||
| if (oc_config.enable_avatars === true) { | ||
| if (shareType === OC.Share.SHARE_TYPE_USER) { | ||
| $('.avatar[data-sharetype="user"][data-sharewith="' + escapeHTML(shareWith) + '"]').avatar(escapeHTML(shareWith), 32); |
There was a problem hiding this comment.
I know we don't have too magic characters in user names, but usually it is best to use .filterAttr('data-sharewith', shareWith') instead. This would solve all possible escaping/injection issues.
There was a problem hiding this comment.
Store $(html) in a variable, for example $shareRow = $(html); then append it to #shareWithList.
Then you can use $shareRow.find('.avatar') without the complex filtering, because you're still working on the same row which only contains a single avatar. Will also be better for performance because it avoids complex lookups.
There was a problem hiding this comment.
You are right. That is much nicer! Fix is on the way.
|
Probably @owncloud-bot is failing because of the forced pushes... otherwise I do not really see a reason. |
|
The inspection completed: No issues found |
|
Yeah... if you see errors like "Could not checkout null" then it's the classic Jenkins fail. |
| html += '</span>'; | ||
| } else { | ||
| html += '<span class="reshare">'+t('core', 'Shared with you by {owner}', {owner: data.reshare.displayname_owner}); | ||
| html += '<span class="reshare">'; |
There was a problem hiding this comment.
Looks like you could move this outside the if block ?
The "span" and avatar "div" are currently repeated.
It seems the only different between the if and else is the text.
|
This is a huge step forward since last time 😄 |
Yeah my javascript fu is coming back bit by bit :P |
* Avatar for "xxxx share with you..." to the left * Avatars for groups and remote shares (use default placeholder) * Modified and added unit tests * Use the same css for all the avatars in the dropdown
|
A new inspection was created. |
|
The inspection completed: 3 new issues |
|
Jenkins is happy in #14679 |
|
Great stuff!
👍 |
|
Needs a second reviewer @jancborchardt @MorrisJobke @nickvergessen |
|
Works 👍 |
Avatars in share dialog fixes
PR for #14457. Should add more consistency to the sharedialog