-
Notifications
You must be signed in to change notification settings - Fork 126
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
Feat: Visual canvas dashboard #6294
Conversation
…te` to template spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, nice progress! I kicked off a UXQA doc here. I'd be fine if we took most of the UXQA items in follow-up PRs, but IMO the code comments would be nice to resolve before merge – else I fear we'll lose them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a second, I thought this file was referring to the canvas itself not a chart/component. I think we should drop the "Canvas" prefix everywhere we write "CanvasComponent".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, could we move features/canvas/components
to features/components
. Ultimately, it should be possible to embed individual components (not requiring a wrapping canvas). Then, at some point, we could migrate toward the long-term ideal of embedding these components into the Explore dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any prefix, I think the prefix should be "DashboardComponent", which would better imply that these components can be embedded into either the Canvas dashboards or the Explore dashboards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regards to moving features/canvas/components
to features/components
-
We previous had a features/templates
folder (still exists but lot of templates have been removed, goal is remove it all in future). It unnecessarily made it complex to use vega charts in TDD and in Canvas. I feel the right approach is to keep building blocks for common components inside src/components
and then they can be used by Canvas or Explore as needed. These components would have to handle two different types of state managers so feels better to have a copy of their own for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, IMO, we consider src/components
as the Rill design system, which, in theory, could be published for anyone to use. Each component should be isolated/encapsulated and should not have dependencies on Rill systems (like a Rill runtime or a Rill app's state management system).
We almost adhere to that ideal today. Except, for example, I see that the src/components/preview-table/ConnectedPreviewTable.svelte
fetches data from a Rill runtime.
So, if the building blocks you're proposing do not include Rill data fetching or Rill state management, then, yeah, I think it's a great idea to put them in src/components
. If they do include Rill data fetching or state management, then I'd vote for putting them in src/features/components
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, ideally we move away from the React provider pattern. E.g., this component could be replaced by a function initializeStateManagers()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using multiple levels of state providers, have combined it into a single one. We still need it as a component as we want to display the spinner when the state is not ready.
web-common/src/features/canvas/components/BaseCanvasComponent.ts
Outdated
Show resolved
Hide resolved
web-common/src/features/canvas/components/ComponentRenderer.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock the merge
This PR creates a base for visual canvas dashboard. Changes specific to UX, component design, layout etc. would be addressed in a follow up PRs.