Fix single button action icon attribute

[PVince81]

Actual:

Expected:

The icon should appear.

It already works with this format when inside the popover, it's only when there's only a single action that it doesn't.

The component is used that way in many places in the server, see nextcloud/server#33508 (comment)

[PVince81]

works now:

the old button on stable5 was rendered directly and using the class https://github.com/nextcloud/nextcloud-vue/blob/stable5/src/components/Actions/Actions.vue#L181-L182

so this fix also passes the icon attribute if the icon slot is not used

[raimund-schluessler]

At least from my side, this is a "planned regression" aka breaking change. When moving the Actions component to a render function in #2911 I didn't implement icon-classes for the single action button icon. This is for two reasons, mainly:

• We use the Button component as the single action trigger and the Button component does not easily support icon-classes but uses an icon slot for material design icons.
• I was under the impression that using icon-classes is discouraged now with the changes in server.

From my side, the proper solution is to populate the icon slot of the ActionElements (i.e. ActionButton, ÀctionLink` etc.), which then makes the single action trigger icon appear. This is more work on the server side, but the "future proof" and clean solution, imho.

The other approach would be to re-implement icon-classes for the Actions menu, but that wouldn't be so nice and a bit hacky.

I would like to hear what @skjnldsv things about this.

[PVince81]

the icon classes still seem to be working when inside the menu, so probably they were not fully removed

from my understanding the reason we wanted the new Actions impl was because it was the only way to also fix the infinite loop related to floating vue, and we definitely want floating vue on the server for accessibility reasons, but don't have time yet to adjust all apps to move away from using icons

so I'm not sure if now is the right time to remove the icon classes ?

let's see what others say

[PVince81]

and thanks for clarifying!

[PVince81]

as we need to move forward a bit quickly, I've merged this now

I've raised #3007 to continue the discussion and to plan the removal of icon support

[raimund-schluessler]

I guess the solution is simple enough to have it implemented, so fine with me. But what still does not work is multiple actions with custom icon class:

<Actions default-icon="icon-edit">
	<ActionButton icon="icon-edit" @click="alert('Edit')">Edit</ActionButton>
	<ActionButton icon="icon-delete" @click="alert('Delete')">Delete</ActionButton>
	<ActionLink icon="icon-external" title="Link" href="https://nextcloud.com" />
</Actions>

[PVince81]

@raimund-schluessler it worked on this PR, this is why I modified one of the examples to use an attr icon, see:

and also it was working on nextcloud/server#33508 (comment) from the dropdown menus which also use icon attributes: https://github.com/nextcloud/server/blob/update-nextcloud-vue-6.0.0-beta.1/apps/files_sharing/src/components/SharingEntryLink.vue#L285

[PVince81]

... and because it was working it made me wonder if something got omitted/forgotten during the Actions port to render function

[PVince81]

bad screenshot, the slot is populated, will do another one

[PVince81]

I've updated the screenshot in #3006 (comment) with the styleguide from master where I unpopulated the slot and use only attr for inside the menu

[raimund-schluessler]

That's not what I mean. You could overwrite the multiple actions trigger icon with the default-icon prop of Actions.
See here:

This should show the icon-edit as menu trigger icon, but it shows the three dot icon instead.

[raimund-schluessler]

I removed the default-icon prop in #2911, because I though it's not desired anymore. You will need to re-add it to make it fully backwards compatible. See here:
https://github.com/nextcloud/nextcloud-vue/blob/601f4208e542ca51b82937c1c1a5c2db25d8b2ab/src/components/Actions/Actions.vue#L299-L307

[raimund-schluessler]

I will provide a PR later today.

[PVince81]

@raimund-schluessler thanks a lot for bringing this up, clarifying and looking forward to your PR :-)