Fix tabs & lint

[skjnldsv]

After #1281

We cannot check for the VNode tag name as we can wrap the Tab and it should be ok.
Otherwise the files app Sidebar doesn't work

That way you can actually provide any tab as long as they're providing the necessary data.

Next step: build it with a render function, would be much cleaner (as we're now have the tabs split from the Sidebar component)

[raimund-schluessler]

We cannot check for the VNode tag name as we can wrap the Tab and it should be ok.
Otherwise the files app Sidebar doesn't work

That way you can actually provide any tab as long as they're providing the necessary data.

I don't get it. A <random-component name="Tab" id="tab1" icon="tab-icon" /> would also be considered a valid AppSidebarTab, although it doesn't implement the tab logic https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/AppSidebarTab/AppSidebarTab.vue#L60-L64 and would never be visible. How is this desired?

[raimund-schluessler]

Also, could you please clean up the history? I think a dependency update commit somehow got into it.

[skjnldsv]

I don't get it. A <random-component name="Tab" id="tab1" icon="tab-icon" /> would also be considered a valid AppSidebarTab, although it doesn't implement the tab logic https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/AppSidebarTab/AppSidebarTab.vue#L60-L64 and would never be visible. How is this desired?

isActive is only here for the Tab, it's not mandatory. It's a helper. It could be removed tbh.
And yes, randomComponent is fine to have.

[skjnldsv]

I think a dependency update commit somehow got into it.

Nope, I included it because there was a conflict in the deps and was preventing the lint to fully works properly. Now it's working and also fixes some lint issues in the AppSidebars components (minor)

[raimund-schluessler]

I think a dependency update commit somehow got into it.

Nope, I included it because there was a conflict in the deps and was preventing the lint to fully works properly. Now it's working and also fixes some lint issues in the AppSidebars components (minor)

Ah, ok. 👍 But now that the lint works again, it shows an error in src/mixins/userStatus.js. Might have been better to fix that separetly 🙈

[skjnldsv]

Ah, ok. But now that the lint works again, it shows an error in src/mixins/userStatus.js. Might have been better to fix that separetly

Fixed in #1304 already ;)

[raimund-schluessler]

isActive is only here for the Tab, it's not mandatory. It's a helper. It could be removed tbh.
And yes, randomComponent is fine to have.

But as far as I understood it, its only fine if there is no AppSidebarTab as mixing tabs and non-tabs is not possible. Could you please point me to the code in server where this fails? I think I don't understand the problem yet.

[skjnldsv]

But as far as I understood it, its only fine if there is no AppSidebarTab as mixing tabs and non-tabs is not possible. Could you please point me to the code in server where this fails? I think I don't understand the problem yet.

https://github.com/nextcloud/server/blob/4485cb30a1bd4b86492203b20c43e0c8fdd47f71/apps/files_sharing/src/views/SharingTab.vue
We need to implement our own logic before the tab. Meaning the Tab is not the direct children but the SharingTab is :)

(This is because of the API implementation)

[raimund-schluessler]

We need to implement our own logic before the tab. Meaning the Tab is not the direct children but the SharingTab is :)

I see. But then you should get rid of this warning https://github.com/nextcloud/nextcloud-vue/pull/1303/files#diff-9d00c73ea5b493b4a9fa09a15ee85430R235 completely, because it has no sense if it isn't enforced.

[skjnldsv]

I see. But then you should get rid of this warning #1303 (files) completely, because it has no sense if it isn't enforced.

I don't even remember why it's here to be honest

[raimund-schluessler]

We need to implement our own logic before the tab. Meaning the Tab is not the direct children but the SharingTab is :)

I see. But then you should get rid of this warning https://github.com/nextcloud/nextcloud-vue/pull/1303/files#diff-9d00c73ea5b493b4a9fa09a15ee85430R235 completely, because it has no sense if it isn't enforced.

Or at least change the string, as the warning is not about mixing tabs and non-tabs, but that a component doesn't have the wanted properties.

[skjnldsv]

Or at least change the string, as the warning is not about mixing tabs and non-tabs, but that a component doesn't have the wanted properties.

No it's not, it's because we allowed devs to provide anything they want if it's only one tab.
so you can put anything without a Tab if you don't provide more than one children. That why that was here.

It's either multiple tabs and you have to use Tabs or one component and you CANNOT use tabs.

[raimund-schluessler]

I don't even remember why it's here to be honest

😆 I don't know either. I just saw that it falsely complains and tried to fix it in #1281

[raimund-schluessler]

No it's not, it's because we allowed devs to provide anything they want if it's only one tab.
so you can put anything without a Tab if you don't provide more than one children. That why that was here.

It's either multiple tabs and you have to use Tabs or one component and you CANNOT use tabs.

Exactly. But wrapping Tabs into another component cancels this check. You cannot distinguish anymore between using multiple tabs or not, because basically anything can be a tab now (as long as it has name, id and icon).

[raimund-schluessler]

I am fine with either, but please adjust the tests, so it is clear what the wanted behavior actually is.

[raimund-schluessler]

And hey, calling #1281 "Bugged" in the release notes is not fair 😀 It did exactly what I thought was the wanted behavior 😉 Sorry for breaking the server ShareTabs stuff 🙈

[skjnldsv]

And hey, calling #1281 "Bugged" in the release notes is not fair It did exactly what I thought was the wanted behavior Sorry for breaking the server ShareTabs stuff

Well, we do need to warn devs of unwanted behaviours :p

[raimund-schluessler]

Not sure if you want this, but the Netlify example now prints a lot of these warning, since this.$slots.default contains entries only consisting of whitespace:

[raimund-schluessler]

Could be fixed by checking if the entry has a tag or non-whitespace text, similar to https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/AppSidebarTabs/AppSidebarTabs.vue#L230:

this.$slots.default.filter(elem => elem.tag || elem.text.trim())

[raimund-schluessler]

It only warns not when the node contains more than only whitespace.
However, it still prints a debug message on the console when you correctly provide a single node and no tabs. But since it is only a debug message it might be ok (although I would prefer to not have this output).
@skjnldsv What do you think?

[raimund-schluessler]

Looks like this for the empty content example (with no tabs and only a single node):

[raimund-schluessler]

Found another issue, needs to be fixed.

[raimund-schluessler]

Good to be merged now, with a minor nitpick. See #1303 (comment).

[skjnldsv]

Maybe we could warn if the $slots.default count is greater than 1?

[raimund-schluessler]

But only after we filtered the default slots for nodes with only whitespace. Maybe we should just push the invalid nodes to a separate array and output it if we found tabs. I will try to write a test function for this in the evening.

[skjnldsv]

Please rebase :)

[raimund-schluessler]

@skjnldsv Rebased, but #1303 (comment) needs to be fixed still.

[georgehrke]

Tested and works

Looks good except for @raimund-schluessler's remark: https://github.com/nextcloud/nextcloud-vue/pull/1303/files#r473747513

[raimund-schluessler]

@skjnldsv I adjusted the tests to check for the console output and adjusted the code to only warn for invalid tabs if there are also valid tabs (mixing tabs and non-tabs content).