-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Table: Add custom cell rendering option #70999
Conversation
/** @alpha */ | ||
subData?: DataFrame[]; | ||
/** @alpha Used by SparklineCell when provided */ | ||
timeRange?: TimeRange; | ||
} | ||
|
||
export interface CustomCellRendererProps { |
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.
This interface may be still up to discussion not sure though if it makes sense to narrow this down (though maybe all you need is frame and index and other parts are just shortcuts) and nothing comes to my mind that should be added.
There may be some perf implications in case somebody wants to memoize the custom component. Then I would assume having just the value
would mean we would not rerender rows/values that did not change, while with this it will rerender because of the dataFrame
but you can mitigate it by custom arePropsEqual
function so I think it's not an issue here.
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.
I think this looks good! (just one issue with the index name prop).
Hold a bit on merging this, discussing with dominik on how best to represent the custom cell component in field config (via stringId that maps to custom cell registry or directly put the component there) |
Think this is important to mind that the problem Andrej is trying to solve with this PR applies to a certain scenario - directly using
@torkelo - I've also discussed this with @aocenas last week. My longer-term idea, to support custom cells in Table Panel was to allow dynamically (runtime) registered cells. With this idea, the required changes that we would have to incorporate will go on top of this PR, so I don't necessarily think we should not proceed with this approach, I think it's gonna be complementary. What's missing in this PR, is the grafana/schema representation of a custom cell. For now, we simply add a new type of a cell: https://github.com/grafana/grafana/pull/70999/files#diff-ac76223ce662d166b6a3b0343bd2755a9faf9e37a2c6ee9c23520216b1f51008R659. In schema we do not specify So what I was thinking about in the future is introducing interface TableCustomCellOptions {
type: TableCellDisplayMode.Custom;
cellComponent: string; // From schema POV this is an ID of a custom cell
} Next, the grafana/ui Tabe component would expose public API The change that would be required to this code is a change in the grafana/ui defined type export interface TableCustomCellOptions {
cellComponent: FC<CustomCellRendererProps>;
type: schema.TableCellDisplayMode.Custom;
} would become: import { TableCustomCellOptions as RawTableCustomCellOptions } from '@grafana/schema';
interface TableCustomCellOptions extends Omit<RawTableCustomCellOptions, 'cellComponent'> {
cellComponent: RawTableCustomCellOptions['cellComponent'] | FC<CustomCellRendererProps>;
type: schema.TableCellDisplayMode.Custom;
} In the custom cell rendering https://github.com/grafana/grafana/pull/70999/files#diff-d083a528bb346e1c0f5af4fc4024973ebba988da9783de5f1907d263d0e745e2R32-R35 we would use the grafana/ui There is a potential problem with where the instance of the registry lives. I think given grafana/ui bundling with plugins, this should not be a show stopper as the runtime registered cells will be "local" to the scope of a plugin, so the correct registry instance should be used. |
|
||
// Can be used to define completely custom cell contents by providing a custom cellComponent. | ||
export interface TableCustomCellOptions { | ||
cellComponent: FC<CustomCellRendererProps>; |
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.
Given my comment I think we should rename this property to something that can express both component and an ID. I would suggest maybe just cell
?
Or alternatively - following up on my comment mentioned above - the grafana/schema type could be:
interface TableCustomCellOptions {
type: TableCellDisplayMode.Custom;
cellId: string; // From schema POV this is an ID of a custom cell
}
and the grafana/ui Table's component type:
import { TableCustomCellOptions as RawTableCustomCellOptions } from '@grafana/schema';
interface TableCustomCellOptions extends Omit<RawTableCustomCellOptions, 'cellId'> {
type: schema.TableCellDisplayMode.Custom;
cellId?: RawTableCustomCellOptions['cellId'] // Mind I'm making this optional here
cellComponent?: FC<CustomCellRendererProps>;
}
I'm on a fence about which one is better. Maybe having a union type is better than extending the interface with additional property cellComponent
.
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.
So I wonder if this is needed. Right now I extend the TableCellOptions
with TableCustomCellOptions
inside the grafana/ui
and don't touch the grafana/schema
. Once this is needed for panel I think TableCustomCellOptions
can be created in grafana/schema
(or table panel) and those two should be separate exclusive types imho. Basically, there is some base in grafana/schema
that is common but then each extends it in its own way to do custom rendering. Otherwise, I feel we would leak the table panel abstractions into table component.
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.
Honestly, I feel like we shouldn't even be importing grafana/schema
into grafana/ui
and should just live with some duplication as even though the types may be same textually, they represent slightly different concepts and have different constraints as seen here.
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.
@aocenas - I am not suggesting changing the type in grafana/schema now - I think we will do this when we decide to provide a custom cell rendering possibility via field configuration. So for now, this is all good.
This PR adds the possibility to define custom renderer function/component for a field in a table through TableCellOptions.
This is piggybacking on Torkels previous PR. You can also see an example implementation of it here
The main part of this is this interface:
Which you can now use like this
This can be used to render values in custom ways if already existing
TableCellOptions
weren't enough or to create "dummy" fields just to show some custom actions per row.