-
Notifications
You must be signed in to change notification settings - Fork 79
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
Custom Sidebar is not put in the sidebarContainer slot #217
Comments
Good find, thanks for sharing! I didn't contemplate people might want to develop their own sidebars. This is a limitation that shouldn't exist. To be honest, the mechanism is a bit hacky as it is, and I think partially matching the string "sidebar" would be even hackier. I'd prefer having something that tells the Page where to put it, whether it's the core Sidebar or a custom component. I'm thinking an additional attribute in the export type StreamsyncComponentDefinition = {
[...]
insertionArea?: "default" | "sidebar";
}; I believe this is cleaner and would allow us to develop e.g. "footer" or "topbar" in the future, without breaking anything. Feel free to send a PR; it'd be appreciated. Should still be quite straightforward I believe. Apart from that change in type as shown above, I think it'd just be checking whether the component is |
Hi @ZBMO , can you please confirm whether you want to work on this PR? Otherwise we're happy to take it ourselves |
@ramedina86 sorry for the delay, I'd like to but I won't be able to work on it till next week. I understand if you need it done sooner. |
@ZBMO no worries at all, next week is fine. And of course no pressure, just want to avoid two people working on it at the same time. |
@raaymax please take care of this which will also help address the complexities around reusing positionless components |
functions for :component-filter check if the c.type == 'sidebar' or c.type != 'sidebar'.
The result is that a custom sidebar isn't placed in the appropriate slot (having its type prepended with 'custom_') and is arranged on the page the other layout components.
a fix would be to change the equality checks on lines 6 and 19 of CorePage.vue to use .includes() instead of ==.
I would like to make a PR for this
The text was updated successfully, but these errors were encountered: