Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NcButton fails to compile without direct text content in default slot #3824

Closed
Antreesy opened this issue Feb 27, 2023 · 4 comments · Fixed by #3827
Closed

NcButton fails to compile without direct text content in default slot #3824

Antreesy opened this issue Feb 27, 2023 · 4 comments · Fixed by #3827
Labels

Comments

@Antreesy
Copy link
Contributor

Antreesy commented Feb 27, 2023

Issue raised because of computed value text() in NcButton, which doesn't consider result of computed value hasText() properly

Template Result
image ---
image image

Proposal to discuss:

replace line:

const text = this.$slots.default?.[0]?.text.trim()

with:

const text = this.$slots.default?.[0]?.text?.trim()
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Feb 27, 2023

@Antreesy Can you check if it works (better) with v7.6.0 for you? In #3726, which is included in the 7.6.0 release, we moved to a render function, which doesn't anymore use the code you show.

In the docs https://nextcloud-vue-components.netlify.app/#/Components/NcButton wrapping the default slot text in a <p>Text</p> doesn't give a runtime error (but also does not show the text, but I think that's expected).

@Antreesy
Copy link
Contributor Author

we moved to a render function, which doesn't anymore use the code you show.

Sorry, @raimund-schluessler, I made a reference from an old version, but in 7.7.1 runtime error still remains (updated example)

(but also does not show the text, but I think that's expected)

Guess, it's the question to the NcButton behavior. I found it quite inconvenient to wrap a child component to <template #icon> and override icon styles, if I need to. Would like to use a default slot for that

@raimund-schluessler
Copy link
Contributor

we moved to a render function, which doesn't anymore use the code you show.

Sorry, @raimund-schluessler, I made a reference from an old version, but in 7.7.1 runtime error still remains (updated example)

Alright, thanks for checking. I guess the warning is only shown in development mode and the docs run in production. Checking if text is undefined is a good idea, I think.

(but also does not show the text, but I think that's expected)

Guess, it's the question to the NcButton behavior. I found it quite inconvenient to wrap a child component to <template #icon> and override icon styles, if I need to. Would like to use a default slot for that

I think I don't really understand what you want to do. Can you show an example of what you intent to do, please? We generally try to keep everything consistent, so overwriting the style of a component should really only be done in edge cases, I would say.

@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Feb 27, 2023

(but also does not show the text, but I think that's expected)

Guess, it's the question to the NcButton behavior. I found it quite inconvenient to wrap a child component to <template #icon> and override icon styles, if I need to. Would like to use a default slot for that

Ah, you mean to just put the content of the default slot where we currently put the text string? That would be fine with me, but I think it collides with the original intention of limiting the way the NcButton component can be used in order to keep things consistent. But that's a question to @marcoambrosini and @skjnldsv, I would say.

Nevertheless, here is the fix for the first part of the issue: #3827

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants