-
Notifications
You must be signed in to change notification settings - Fork 93
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
Button! #1808
Button! #1808
Conversation
98ebcc8
to
a14c272
Compare
This comment has been minimized.
This comment has been minimized.
a14c272
to
8de32a5
Compare
This comment has been minimized.
This comment has been minimized.
8de32a5
to
8b54c05
Compare
8b54c05
to
deaf581
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
46bb7cb
to
6838462
Compare
7570ff9
to
516e5d1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Let's call it
No worries, that's on me :) |
I thought about this again and I think one can just import/mount the Button component under a different name in case there is a clash between the button tag and the Button component. So I guess it's a matter of taste if we rename it or not. It should work either way. |
Signed-off-by: marco <marcoambrosini@pm.me>
516e5d1
to
d441b9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good
I guess this will be in nextcloud/vue@4.3.0 since it is not a breaking change. But due to nextcloud/server#25774 using the Button component will require Nextcloud 22.2.1 or higher. How do we handle this, and could we maybe backport nextcloud/server#25774 to stable21 and stable20 as well (since they are still supported)? |
This new component doesn't work with older versions of Nextcloud. This is a breaking change IMO. I would make a clear cut with a major release. |
But it also doesn't break anything for apps that upgrade, since the component simply wasn't available before 🙈. I guess it's a matter of interpretation under which version number we release it. The more severe problem is which server version will be compatible. If we really don't backport, many apps (e.g. Calendar, Tasks, Maps, Contacts, all which support all server versions with a single release) will only be able to use the button component (or strictly speaking, updating to nc-vue@5.0.0) once server v23 is the oldest supported version (July 2022). This will make the component quite useless until then. Is there maybe the possibility to make the Button component self-contained (i.e. not dependent on a specific server version), by including all CSS properties and variables in nc-vue? That way, it could be used with any server version. |
It heavily relies on the theming engine in the server. This is something we will encounter every time we'll have to add new Colors, I don't think that bringing everything to the component's scoped styles is a good solution. |
That's up to Nextcloud to decide internally, I guess. It just means that this component is mainly useless until server v22 is EOL. |
Requires nextcloud/server#25774
Test with nextcloud/spreed#6121