NcButton: Allow to format text of button

[susnux]

☑️ Resolves

Allow us to format the text of a button without dirty hacks like :deep selectors, I added the example why this is useful and where we should use this: Table headers.

Places where we have this and can remove all custom hacks are: all file tables in the files app, the logreader app, the file picker.

⚠️ this is chained on #4366 (see first commit), should be reviewed and merged first

🖼️ Screenshots

🏁 Checklist

• 📘 Component documentation has been extended, updated or is not applicable

[jancborchardt]

Compared to the normal buttons it seems like the text is a bit higher up here than there. Might be a non-issue since it’s only visible on hover / with background.

[marcoambrosini]

Same as for trailing icons, unless we have other use cases I would wrap the button component into a tableHeaderButton or something like that and make the style changes from there

[susnux]

Compared to the normal buttons it seems like the text is a bit higher up here than there. Might be a non-issue since it’s only visible on hover / with background.

Tested this with @szaimen on Window, it depends on your local font, we have a hack (1px margin) to center it with Segoe font which is aligned differently, but it breaks on other fonts (like I am using Roboto). See also #4200

[ShGKme]

Same as for trailing icons, unless we have other use cases I would wrap the button component into a tableHeaderButton or something like that and make the style changes from there

We have many buttons in Talk with custom text styles: LeftSidbar, Settings button, Conversation, TopBar, CallTime and etc.

[ShGKme]

I agree with this change to allow custom content of the button (otherwise it should be a prop, not a slot).

But I'm not sure if it is the best way to customize text styles.

With this change, the usage of custom styles requires creating a new element just to set class:

<template>
  <NcButton class="my-button">
    <!-- There is no meaning in this span, only for setting a text's color -->
    <span class="my-button__text">Text</span>
  <NcButton>
</template>

<styles scoped>
.my-button__text {
  color: red;
}
</styles>

I'd prefer to have a prop to set inside styles or classes:

<template>
  <NcButton class="my-button" text-classes="my-button__text">
    Text
  <NcButton>
</template>

<styles scoped>
/* Also deep, but for my own classes, not NcButton's */
.my-button :deep(.my-button__text) {
  color: red;
}
</styles>

Or

<template>
  <NcButton class="my-button" text-styles="color: red">
    Text
  <NcButton>
</template>

[susnux]

I like the idea of having text-classes property, but than we should also deprecate the default slot. As than it is a misuse of Vue slots.
We probably should move to a text property instead.

[raimund-schluessler]

Once this is merged, one way or the other, we could then also adjust these lines

nextcloud-vue/src/components/NcBreadcrumb/NcBreadcrumb.vue

Lines 339 to 345 in 74a7e28

	.button-vue {
	padding: 0 4px 0 16px;
	&__wrapper {
	flex-direction: row-reverse;
	}
	}

[susnux]

The more I think about this, the more I think we should went with this one.

Why? Because we use the pattern of the default slot = content everywhere. Only moving the Button to text property seems non consistent, while I personally prefer that idea.
Moreover the HTML <button> also allows every other content inside, so it feels more native.

[skjnldsv]

I still don't get it. You can just set a class to the button and change the text styling? deep only works because there is a scoped css being used, but you are free to use standard css? I don't think we actually needs this, using :deep is not a hack, it's one way to ensure your css isn't scoped within a scoped <style>. Feel free to just use standard css?

I think we should keep things simple :)

Once this is merged, one way or the other, we could then also adjust these lines

src/components/NcBreadcrumb/NcBreadcrumb.vue#L339-L345

What does this do?
Put the icon at the end?
You can just use direction: rtl to adjust the text order and stay accessible

[susnux]

Basically the problem is: Either we should allow to put any content inside the button, like the native html button allows, including styling. Or we should add a way to apply custom styles or classes to the text wrapper.
Currently you can only apply a class to the whole button and then have to assume internals of the button component, see below.

using :deep is not a hack, it's one way to ensure your css isn't scoped within a scoped <style>

Yes but then you need to assume some internals of the button, and this is what you should not do.
Components should be a black box so that you do not depend on its implementation (because the class is assigned to the root element and not to the text wrapper).

You can just use direction: rtl to adjust the text order and stay accessible

No this is quite bad as you misuse it for stlying while you should use if for localization.

---

So a overall problem is missing consistancy with the components architecture. Sometimes you use the default slot for content and allow child nodes, sometimes a property. But for the button we use the slot but only allow text.
Meaning to be consistent we should either only allow text and then use a property together with a way to format that text, like the suggestes text-classes property, or allow any content within the button and simply use the default slot like it is done here.

[skjnldsv]

To make sure I get it right, the use case (not talking about the wider discussion) is just about reveting text and the icon, right?

[susnux]

To make sure I get it right, the use case (not talking about the wider discussion) is just about reveting text and the icon, right?

No that is already implement in 8.x by using the alignment variable.

It is about formatting the text, like color or weight. See the example added, it then can be used for the files list header (e.g. in files app or file picker) or for the log reader header. Simply speaking for every table header without to have using any hacks that depend on private internals of the component.

[susnux]

The more I think about this, the more I think we should went with this one.

After seeing all that validator issues @ShGKme fixes at the moment, maybe go with the alternative PR[1]. So that we do not allow arbitrary (and possibly wrong) content of the button but only text (and the icon).

What do you think?

[1] https://github.com/nextcloud-libraries/nextcloud-vue/pull/4393

[ShGKme]

deep only works because there is a scoped css being used, but you are free to use standard css? I don't think we actually needs this, using :deep is not a hack, it's one way to ensure your css isn't scoped within a scoped <style>. Feel free to just use standard css?

I think we should keep things simple :)

deep is a hack because it allows changing component's internal implementation (until we explicitly say that some class name is a part of the component's public interface and a way to customize it).

With deep any simple layout/class name refactoring brakes a layout.

[susnux]

As this seems to get more positive feedback than the alternative #4393

Shall we go with this one? 👍 | 👎 ?

[jancborchardt]

@raimund-schluessler since you commented on the other one, what is your take here – approved or any feedback? :)

[susnux]

Well then lets go with this one as it is less breaking?