-
Notifications
You must be signed in to change notification settings - Fork 8
Migrate the sidebar tab to the new API #355
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
Conversation
Signed-off-by: Julien Veyssier <[email protected]>
susnux
left a comment
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, putting the t method into data is a bit weird, would rather put it into methods or setup.
But otherwise 👍
src/components/Info.vue
Outdated
| data() { | ||
| return { | ||
| t, |
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.
this will make the method reactive, probably better to move this to setup()
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.
I want to stick with the options API so it's been moved in methods.
It seems like there is no way to inject those functions in a mixin of a component defined with defineAsyncComponent. Do you confirm?
It would be much more convenient to inject the translation functions this way like we usually do.
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.
I want to stick with the options API so it's been moved in methods.
There is no conflict between options and composition API both can exist in parallel :)
It seems like there is no way to inject those functions in a mixin of a component defined with defineAsyncComponent. Do you confirm?
I guess you mean like the global mixin for the vue instance? No that is not possible (but not because of async component but because of the web component).
What you could do it put it into a legacy mixin but I would strongly discourage using that as mixins in general already deprecated in Vue in favor of composables.
src/approvalTab.js
Outdated
| } | ||
| */ | ||
| // setup tab | ||
| setupTab() |
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.
you can also do this globally outside of enable as with Vue 3 the async component works fine. but does not harm.
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.
Like this for example? 5f0d3b1
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.
yes :)
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
closes #354
OCA.Files.Sidebarwas removed in Nextcloud 33.registerSidebarTabfrom@nextcloud/fileserror registerSidebarTab not found in '@nextcloud/files' import/nameddefineAsyncComponentso I had to import the translation functions in all sub components of the sidebar tab.