Add ListItemIcon component

[skjnldsv]

It was about time we have one of those. 🙄

@ma12-co @PVince81 It seems to be pretty much the same design as in Talk (except you're using 44px size, which I implemented), please tell me if there is a hidden feature on talk that I might have missed here :)

---

[skjnldsv]

Ping everyone :)

[marcoambrosini]

I think that the second line should be allowed only if the avatar is >= 44px

[skjnldsv]

I think that the second line should be allowed only if the avatar is >= 44px

32px still looks good.
Put I think you have a point, I'll add 32px as min for now (that is what we use in lots of places for now.

[skjnldsv]

@ma12-co

[ChristophWurst]

Srsly best PR I've seen in the last 10 mins

[PVince81]

I don't really understand the purpose of supplying a subtitle if that one is going to be hidden due to small size. Should it appear in a tooltip ? Should screen readers read it ?

[PVince81]

I've tested it with the screen reader and as long as the text is displayed it will be read out.
And also tested that it's possible to specify an aria-label + role on the element in case one wants to do role="listitem" aria-label="the item label to be read out"

[PVince81]

will this require changes on the consumer side ? probably everyone will need to switch to ListItemIcon now ?

when compiling this branch and linking it with server, the share multiselect results are broken:

[skjnldsv]

when compiling this branch and linking it with server, the share multiselect results are broken:

Yes, this require changes on the object
This is a breaking change.

[skjnldsv]

I don't really understand the purpose of supplying a subtitle if that one is going to be hidden due to small size. Should it appear in a tooltip ? Should screen readers read it ?

I'm opened to suggestions :)
We could do the tooltip if there isn't enough room for sure!

[PVince81]

I don't really understand the purpose of supplying a subtitle if that one is going to be hidden due to small size. Should it appear in a tooltip ? Should screen readers read it ?

I'm opened to suggestions :)
We could do the tooltip if there isn't enough room for sure!

or maybe whoever is passing in a small size should simply not set the subtitle ?
or is it intended to be responsive ?

I guess a tooltip could be a good approach if the decision whether to hide the subtitle is done by the component.

[skjnldsv]

or maybe whoever is passing in a small size should simply not set the subtitle ?
or is it intended to be responsive ?

No it's not.
It's more like a fool proof concept tbh.
That way I'm sure no one will ^^

Expecting random conditions to be read by devs is a no-no from me, never works 🙈

[PVince81]

as a heads up, after merging we'll need to recheck and adjust the following consumers of Multiselect:

I've grepped for <Multiselect

from git:

apps/files/src/components/TransferOwnershipDialogue.vue
apps/files_sharing/src/components/SharingInput.vue
apps/settings/src/components/UserList/UserRow.vue
apps/settings/src/components/AdminTwoFactor.vue
apps/settings/src/components/AppDetails.vue
apps/settings/src/components/UserList.vue
apps/settings/src/views/Users.vue
apps/updatenotification/src/components/UpdateNotification.vue
apps/workflowengine/src/components/Checks/FileSystemTag.vue
apps/workflowengine/src/components/Checks/MultiselectTag/MultiselectTag.vue
apps/workflowengine/src/components/Checks/RequestTime.vue
apps/workflowengine/src/components/Checks/RequestURL.vue
apps/workflowengine/src/components/Checks/RequestUserAgent.vue
apps/workflowengine/src/components/Checks/FileMimeType.vue
apps/workflowengine/src/components/Checks/RequestUserGroup.vue
apps/workflowengine/src/components/Event.vue
apps/workflowengine/src/components/Check.vue
apps/user_status/src/components/ClearAtSelect.vue
apps/spreed/src/components/AdminSettings/AllowedGroups.vue
apps/spreed/src/components/AdminSettings/GeneralSettings.vue
apps/spreed/src/components/AdminSettings/SIPBridge.vue
apps/spreed/src/components/MediaDevicesSelector.vue
apps/spreed/src/components/RightSidebar/Matterbridge/MatterbridgeSettings.vue
apps/spreed/src/views/FlowPostToConversation.vue

and probably more in apps, here's something from the NC 20 tarball:

apps/privacy
apps/contacts
apps/mail

I wonder if we should adjust first before merging, or at least have the matching PRs ready to merge first ?
Another idea would be to split the Multiselect adjustment to another PR to not delay ListItemIcon.

Thoughts ?

[skjnldsv]

I wonder if we should adjust first before merging, or at least have the matching PRs ready to merge first ?
Another idea would be to split the Multiselect adjustment to another PR to not delay ListItemIcon.

Thoughts ?

It should only be a breaking change for the one using the userSelect prop.
Not just Multiselect. Standard select haven't change :)

[skjnldsv]

https://github.com/nextcloud/server/blob/28e2551313062e04748b32433648bc0177c4a54b/apps/files_sharing/src/components/SharingInput.vue#L36
https://github.com/nextcloud/forms/blob/bd641c53e48cdd12b07f0c7f749a85ea804949e1/src/components/ShareDiv.vue#L36
https://github.com/nextcloud/contacts/blob/b47082b0baf162be7647399276432054856b7716/src/components/Settings/SettingsAddressbookShare.vue#L34
https://github.com/nextcloud/deck/blob/413bf7e2c9573a0201b2429774b8cddd9e30123c/src/components/board/SharingTabSidebar.vue#L8
https://github.com/nextcloud/calendar/blob/4d0962a458a09846f38b637ce67c69a06a93b28a/src/components/AppNavigation/CalendarList/CalendarListItemSharingSearch.vue#L35
https://github.com/nextcloud/server/blob/24e58947d6fba02c3d0ff709213f527411fcec0c/apps/files/src/components/TransferOwnershipDialogue.vue#L54
https://github.com/nextcloud/tasks/blob/a1cf750fa74b83b94163e5df9517218507f27bda/src/components/AppNavigation/CalendarShare.vue#L42
https://github.com/nextcloud/polls/blob/7621e51c3930e442d8ac915e7e35c653364d9864/src/js/components/SideBar/SideBarTabShare.vue#L54
https://github.com/nextcloud/deck/blob/d7d8e707ca29292b04a2e89ab345d6f84ee0c3a3/src/components/card/CardSidebarTabDetails.vue#L63
https://github.com/nextcloud/bookmarks/blob/38a373ada6e139330b49ed10ab1439c1cadce928/src/components/SidebarFolder.vue#L29

Is what I found so far :)

[PVince81]

new results, based on grep -lre "user-select" apps lib core | xargs grep -lre '<Multiselect' on server master branch:

apps/files/js/dist/personal-settings.js.map
apps/files/src/components/TransferOwnershipDialogue.vue
apps/files_sharing/src/components/SharingInput.vue
apps/settings/js/vue-settings-admin-security.js
apps/workflowengine/js/workflowengine.js

and NC 20 release tarball:

apps/updatenotification/js/updatenotification.js.map
apps/contacts/js/contacts-main.js.map
apps/mail/js/mail.js.map
apps/spreed/js/admin-settings.js.map
apps/spreed/js/flow.js.map
apps/spreed/js/talk.js.map

so a bit less, but still a bunch to adjust and test

[skjnldsv]

so a bit less, but still a bunch to adjust and test

So, To adjust the code in your apps, you need to provide title (most likely you want the same as the displayName prop) and replace desc by subtitle
We also stopped providing the status. Just insert it yourself at the subtitle prop :)

[skjnldsv]

@PVince81 I'll let you decide if you're good with this :)

[PVince81]

I'm mostly worried about who will fix all these and how much time we'll have an "unstable" feature in those places.

I come from a place where we'd try and keep master branches as stable as possible to be able to release any time and not have surprises. But the latter involves keeping PRs running for longer times and opening PRs in all places that need adjustment, and then merging all at a given point when all are ready.

Or we open a server ticket where we list all those places and tag as "regression to fix before NC 21 final" and move forward.

[skjnldsv]

I come from a place where we'd try and keep master branches as stable as possible to be able to release any time and not have surprises. But the latter involves keeping PRs running for longer times and opening PRs in all places that need adjustment, and then merging all at a given point when all are ready.

Or we open a server ticket where we list all those places and tag as "regression to fix before NC 21 final" and move forward.

Well, it's not like it's instantly available for all apps.
Once it will be released, dependabot will show the changelog. It's up to the devs to do the required steps to a new major upgrade anyway. Releasing will not break anything until someone upgrade the dependency.

[PVince81]

I come from a place where we'd try and keep master branches as stable as possible to be able to release any time and not have surprises. But the latter involves keeping PRs running for longer times and opening PRs in all places that need adjustment, and then merging all at a given point when all are ready.
Or we open a server ticket where we list all those places and tag as "regression to fix before NC 21 final" and move forward.

Well, it's not like it's instantly available for all apps.
Once it will be released, dependabot will show the changelog. It's up to the devs to do the required steps to a new major upgrade anyway. Releasing will not break anything until someone upgrade the dependency.

Good point, I missed that. Thanks for the clarification.
Then we can safely merge and take care of adjusting as part of the lib update.

[JBScoutBerlin]

Hello @ALL,

the name of this component is misleading. As the documentation to this component says: "This is used to display a avatar-title/subtitle + icon layout" so the name should be ListUserIcon or ListAvatarIcon to make clear that this is not a generic icon component to be used in lists ( to bad ad all ).

[skjnldsv]

Well, it's because you can display plenty of other things than users. Groups, contacts, simple item :)
ListAvatarIcon is fine for me tbh, Unfortunately we just released. What do you think @PVince81 ?

[PVince81]

or adjust the documentation to make it clearer that you can also build items without icons ?