Canvas: Override dimension filters in panel #6486
Conversation
| const timeAndFilterStore = canvasEntity.createTimeAndFilterStore( | ||
| metricsViewName, | ||
| { | ||
| timeRangeStore: selectedTimeRange, | ||
| overrideTimeRange: overrideTimeRange, | ||
| componentTimeRange, | ||
| componentFilter, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
I'm thinking these selectors are doing too much. Remember, we should be able to embed a single Component without having a Canvas wrapper. I think the selector should not do the resolution of Canvas-level & Component-level state, but rather that resolution should happen outside this function, and the resolved time range & filter should be passed-in. Thoughts?
There was a problem hiding this comment.
I am not sure what is the the timeline to have these components as standalone ones without canvas (if any). I would not consider this a blocker for this PR though. What is the use case where we want these components to be without a canvas wrapper?
There was a problem hiding this comment.
Some use cases:
- Dogfooding, we may embed a single component (e.g. BigNumber or LineChart) for our Rill Cloud usage monitoring feature.
- Our customers may very likely want to do their own single-component embeddings into their applications. (Or, devil's advocate, they'll use a Rill API and hand-roll their own component.)
- Ideally we refactor the Explore dashboard to be built on these components, and we do this migration incrementally one component at a time.
But beyond the use cases, decoupling Components from Canvas will also make the code more maintainable. It'll lessen the cognitive overhead required to edit Component code & Components will be easier to test. As an exercise, it should be trivial to create a Storybook story for all of these Components.
For inspiration, look at the APIs for Evidence's components. They're pure components: all state is passed in, there's no closure over global state.
There was a problem hiding this comment.
My comment below...
There should be one instance of CanvasFilters per Canvas.
... also applies here. Let's assume the case where there are no Component-level overrides. We shouldn't redundantly create a Canvas timeAndFilterStore for each Component.
| export class CanvasComponentState { | ||
| name: string; | ||
| filters: CanvasFilters; | ||
| private spec: CanvasResolvedSpec; |
There was a problem hiding this comment.
Here too, do we need to couple Canvas state with Component state? Given CanvasEntity already holds the CanvasFilters & CanvasResolvedSpec objects, it seems redundant to put them in this class too. I'd expect a ComponentState class to expose a setFilters method, so the Canvas/Component filters can be resolved externally.
There was a problem hiding this comment.
The component filters and canvas filters are two different things. The component filter are preserved in the YAML and are an independent property where global filters is similar to what we have in explore today. As such, we need to maintain 1 copy for global canvas and 1 copy for each of the components where filters can be added
There was a problem hiding this comment.
Yeah, I get that they're two different things, but I don't understand the "copy" part. I'm 1. concerned about coupling Canvas + Components, and 2. it's redundant to instantiate a new CanvasFilters object for every Component. There should be one instance of CanvasFilters per Canvas.
There was a problem hiding this comment.
Point 1 is a bit broader and would be good to discuss this in a larger group.
With regards to point two, we need to preserve different values of filters across global canvas and multiple components.
// For global canvas state
const globalFilters = new CanvasFilters(globalWhere, globalDTF)
const component1Filters = new CanvasFilters(component1Where, componenet1DTF)
const component2Filters = new CanvasFilters(component2Where, componenet2DTF)
...
Do you mean that we should put these component filters inside a single CanvasFilters instead? Wouldn't that lead to even more tighter canvas-component coupling?
There was a problem hiding this comment.
Re: 1, happy to discuss this more, it is a core design decision. FWIW, I was curious what Metabase, Grafana, and Superset do. Here's a quick ChatGPT convo that implies they each follow a "parent-unaware"/decoupled design, and it advocates the same.
Re: 2, agree that Components should have their own filter state, but I wouldn't call these Canvas filters (which implies coupling). What if we did a small refactor to consider CanvasFilters as Filters, then we could use instances of this store for the global Explore filter, for the global Canvas filter, and for individual Component filters. Thoughts on that?
Merge after #6470