Skip to content

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

Merged
skjnldsv merged 1 commit into
masterfrom
revert-445-sidebar-tab-validation
Jun 26, 2019
Merged

skjnldsv merged 1 commit into
masterfrom
revert-445-sidebar-tab-validation

Conversation

@skjnldsv

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@korelstar

Copy link
Copy Markdown
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

skjnldsv commented Jun 29, 2019 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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