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

[NcAppSidebar] v-show doesn't work on sidebar anymore because of Fragment #5623

Closed
ShGKme opened this issue May 20, 2024 · 3 comments · Fixed by #5627
Closed

[NcAppSidebar] v-show doesn't work on sidebar anymore because of Fragment #5623

ShGKme opened this issue May 20, 2024 · 3 comments · Fixed by #5627
Assignees
Labels
bug Something isn't working feature: app-sidebar Related to the app-sidebar component high High priority regression Regression of a previous working feature

Comments

@ShGKme
Copy link
Contributor

ShGKme commented May 20, 2024

As it is in general not good to have UI component not representing DOM node in render, I'm adding a new wrapper over sidebar that includes both the sidebar and toggle.

@ShGKme ShGKme added bug Something isn't working high High priority regression Regression of a previous working feature feature: app-sidebar Related to the app-sidebar component labels May 20, 2024
@ShGKme ShGKme self-assigned this May 20, 2024
@susnux
Copy link
Contributor

susnux commented May 20, 2024

I see. But v-show makes not much sense on NcAppSidebar, it leads to bugs on nextcloud-vue < 8.12 and for 8.12+ you should use v-if or open prop

@ShGKme
Copy link
Contributor Author

ShGKme commented May 21, 2024

But v-show makes not much sense on NcAppSidebar, it leads to bugs on nextcloud-vue < 8.12

I'm not aware of bugs, it worked fine and used on Talk for a very long time 👀

Moreover, v-if is currently not possible to use in Talk and it wouldn't be easy to fix in a short time before the release.

So, I'm changing the structure.

@ShGKme
Copy link
Contributor Author

ShGKme commented May 21, 2024

Or, you mean, that there is an issue with the focus trap on mobile? Then yes, it should not be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature: app-sidebar Related to the app-sidebar component high High priority regression Regression of a previous working feature
Projects
None yet
2 participants