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

Revert "AppSidebar: fix tab validation: tabs are optional" #447

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

skjnldsv
Copy link
Contributor

Reverts #445

@skjnldsv skjnldsv merged commit cf04f7a into master Jun 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the revert-445-sidebar-tab-validation branch June 26, 2019 12:22
@skjnldsv
Copy link
Contributor Author

Hey @korelstar
There was an issue with your fix. If you're not using the appsidebarTab component, then the tabs are always empty, so using reduce on an empty array will never check anything :)

@skjnldsv
Copy link
Contributor Author

Could you open again a fix so we can have a better look at it? :)
You're awesome! Tanks! 🤗

@korelstar
Copy link
Contributor

What exactly didn't work and why should it work? It is intended, that non-AppSidebarTab's are not shown as tabs but as normal content.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jun 29, 2019

Issue is that you can use other components as tabs, as long as you're providing the appropriate icon and name and id. Now if you check the tab component only, it will forbid it :)

(sorry about the late answer)

You can see the current pending pr that broke after your patch : nextcloud/server#15719
https://github.com/nextcloud/server/pull/15719/files#diff-e230d0e08bd5100b6e1bb2626181ff6b
😉

@korelstar
Copy link
Contributor

Okay, then I suggest to make the check as you wrote: a children is treated as tab if it has icon, id and name, otherwise it is treated as non-tab content. However, then your detailed warnings are not possible anymore.

The only other solution I see is to revert #420, since that enforces tabs, which is not intended.

@skjnldsv
Copy link
Contributor Author

I'll adapt your pr and 420 :)
Thanks for the help @korelstar ! 🤗

PS: are you coming at the conference this year? 🚀

@skjnldsv skjnldsv modified the milestone: next Jul 10, 2019
@korelstar
Copy link
Contributor

What about using different template slots for tabs and for non-tab-content? Then, it would be easy to check if and warn if not only one of them is used. I think that would provide a much clearer distinction between these two use cases. Unfortunately, this would be a breaking change (slot has to be used for tabs).

What do you think, @skjnldsv ?

@korelstar
Copy link
Contributor

I'll adapt your pr and 420 :)

@skjnldsv Any news on this? There is still an error message when using no tabs in AppSidebar. Are you fine with slitting tabs and non-tab content in two different template slots? Then I could provide a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants