Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Feb 21, 2025

Required for nextcloud/server#48527

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews labels Feb 21, 2025
@susnux susnux requested review from Pytal, artonge and skjnldsv February 21, 2025 16:11
Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also work with a buttonType prop and pass it as is?

@susnux
Copy link
Contributor Author

susnux commented Feb 22, 2025

Could also work with a buttonType prop and pass it as is?

IMHO to many options as there is no sense in making it error or warning state.

@artonge
Copy link
Contributor

artonge commented Feb 24, 2025

I think I agree with Pytal, even if there is no reason for now, it also lowers the complexity by making it transparent what we are doing under the hood.

@susnux
Copy link
Contributor Author

susnux commented Feb 24, 2025

I think I agree with Pytal, even if there is no reason for now, it also lowers the complexity by making it transparent what we are doing under the hood.

Not sure as then some will use weird combinations which do not want. We want consistent design across all apps so we should limit what dev can do here to sane options.

@artonge
Copy link
Contributor

artonge commented Feb 24, 2025

so we should limit what dev can do here to sane options.

I don't think I agree here, and in any case, devs can still bypass the property by overriding the CSS.
A sane default is enough in my opinion, and we if you really want to limit options, we can hard code the possible values.

@skjnldsv
Copy link
Contributor

I do get both side, and i'll raise another reason to go this way: if the vue button API changes, we suddenly have to also update this API here.
While keeping a boolean, we get to decide how it will impact the uploader design.

devs can still bypass the property by overriding the CSS.

If a dev decide to go this way, they'll already lost.
We can't control everything, but devs should know that if things are getting difficult for what they're trying to achieve, they're probably doing it wrong. 😉

@skjnldsv skjnldsv merged commit 305a93b into main Feb 25, 2025
20 checks passed
@skjnldsv skjnldsv deleted the feat/primary-menu branch February 25, 2025 09:27
@skjnldsv skjnldsv mentioned this pull request Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants