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

[next] feat(NcActions*): migrate to vue 3 #4646

Merged
merged 7 commits into from
Oct 31, 2023
Merged

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Oct 13, 2023

☑️ Resolves

This migrates the NcActions component to vue 3.

@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews feature: actions Related to the actions components vue 3 Related to the vue 3 migration labels Oct 13, 2023
@raimund-schluessler raimund-schluessler added this to the 9.0.0 next Vue 3 milestone Oct 13, 2023
@raimund-schluessler raimund-schluessler changed the title feat(NcActions*): migrate to vue 3 [next] feat(NcActions*): migrate to vue 3 Oct 13, 2023
@raimund-schluessler raimund-schluessler marked this pull request as ready for review October 13, 2023 19:40
@raimund-schluessler
Copy link
Contributor Author

Rebased.

@raimund-schluessler raimund-schluessler force-pushed the feat/2154/actions branch 2 times, most recently from ffb7614 to e117273 Compare October 17, 2023 17:37
@raimund-schluessler
Copy link
Contributor Author

Resolved conflicts.

@raimund-schluessler
Copy link
Contributor Author

Another review would be very welcome 🙂

Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler
Copy link
Contributor Author

Rebased and resolved conflicts.

src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
Comment on lines 25 to +29
before() {
// all actions requires a valid text content
// if none, forbid the component mount and throw error
if (!this.$slots.default || this.text.trim() === '') {
Vue.util.warn(`${this.$options.name} cannot be empty and requires a meaningful text content`, this)
if (!this.$slots.default() || this.text.trim() === '') {
warn(`${this.$options.name} cannot be empty and requires a meaningful text content`, this)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this part work? o_O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What exactly do you mean? I didn't write it, originally, I only adjusted it for vue 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed that it was an old part. I just don't understand how it works in general, who and where call this before() custom option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also find it a bit weird. Every Action* component, e.g. ActionButton uses this mixin to check that either text or the default slot are present.

src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
@raimund-schluessler
Copy link
Contributor Author

@ShGKme Could you review again, please? I hope I fixed everything.

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Oct 30, 2023

@nextcloud-libraries/developers Honestly, it is a bit demotivating that very few seem to care that the components finally support vue 3.

And at the risk of repeating myself: vue 2 reaches EOL in 45 business days. After that, it won't receive any more updates. Even if the risk for security issues in vue 2 is likely really low, it doesn't seem to be the best approach to not give developers at least the option to use supported libraries (especially for a project that always emphasizes to run up-to-date software).

@raimund-schluessler raimund-schluessler merged commit dc5cf6e into next Oct 31, 2023
15 checks passed
@raimund-schluessler raimund-schluessler deleted the feat/2154/actions branch October 31, 2023 05:50
@susnux susnux mentioned this pull request Jan 23, 2024
1 task
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 feature: actions Related to the actions components vue 3 Related to the vue 3 migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants