Datagrid Panel: Edit data within your dashboards #66353
Conversation
…umn/rows, refactors
| const [columnWidths, setColumnWidths] = useState<Map<number, number>>(new Map()); | ||
| const [columns, setColumns] = useState<SizedGridColumn[]>([]); | ||
| const [contextMenuData, setContextMenuData] = useState<DatagridContextMenuData>({ isContextMenuOpen: false }); | ||
| const [renameColumnInputData, setRenameColumnInputData] = useState<RenameColumnInputData>({ isInputOpen: false }); | ||
| const [gridSelection, setGridSelection] = useState<GridSelection>(EMPTY_GRID_SELECTION); | ||
| const [columnFreezeIndex, setColumnFreezeIndex] = useState<number>(0); | ||
| const [toggleSearch, setToggleSearch] = useState<boolean>(false); | ||
| const [isResizeInProgress, setIsResizeInProgress] = useState<boolean>(false); |
There was a problem hiding this comment.
I would try to find a way to move state and logic outside the react componet (using useReducer, or StateManagerBase) or external hook. Easier to unit test state and callback logic and makes it easier to reason about the react rendering logic
There was a problem hiding this comment.
don't forget unit tests for such a big and complex component (and state logic)
There was a problem hiding this comment.
I agree with you on state logic. Testing is underway for this, but I wanted to get eyes on it as soon as possible.
| [FieldType.other, GridColumnIcon.HeaderReference], | ||
| ]); | ||
|
|
||
| if (!frame) { |
There was a problem hiding this comment.
I would move this early return to before the typeToIconMap declaration, that way, we don't define something that possibly won't be used anyway.
|
|
||
| const value = field.values.get(row); | ||
|
|
||
| switch (field.type) { |
There was a problem hiding this comment.
Have you considered using an object here instead? iirc switches run linearly, and object key lookups execute in constant time. Plus, I think it'd be easier to test/maintain? Just a thought.
There was a problem hiding this comment.
interesting, would be curious to see some benchmarks showing this.
There was a problem hiding this comment.
I looked more into it, maybe lookups are NOT faster in modern browsers? -> https://jsben.ch/JYZLQ
I'm surprised!
There was a problem hiding this comment.
I'm surprised!
that's the lesson here. any benchmark that's > 6mo old has to be re-evaluated. JITs improve all the time. i've seen a lot 🤯 code because some old benchmark showed that the straightforward code was slower 2yrs ago.
There was a problem hiding this comment.
actually i think the slowness here is mostly from the lookup version doing function calls, which will always add overhead vs basic constructs like switch and for (vs forEach)
mdvictor
removed
datasource/InfluxDB
datasource/Graphite
area/dashboard/templating
datasource/Elasticsearch
datasource/Prometheus
datasource/CloudWatch
datasource/Postgres
datasource/MSSQL
|
Backend code coverage report for PR #66353 |
|
Frontend code coverage report for PR #66353
|
There was a problem hiding this comment.
@torkelo, @zoltanbedi found a couple things I would appreciate your input on.
| if (typeof toBeConverted !== 'string') { | ||
| toBeConverted = String(toBeConverted); | ||
| } | ||
|
|
There was a problem hiding this comment.
There are situations where values are not string in a string field type. e.g: when drag/dropping and csv the dataframe string fields can have both numeric and strings, thus the need for a conversion here. Not sure if fixing it in the convert transformer is the way to go, I think a better approach is to try to convert values when importing, maybe.
There was a problem hiding this comment.
refactored this a bit
There was a problem hiding this comment.
I'm okay with this. Would be nice to have a test for your use case as well.
|
|
||
| newFrame.fields.map((field) => { | ||
| field.values = new ArrayVector([...field.values.toArray(), null]); | ||
| }); |
There was a problem hiding this comment.
The field values have been modified recently and the received field values aren't ArrayVectors anymore, but ArrayPropertyVector's. I'm not sure why it has changed but the entire datagrid is built around arrayVectors and changing the functionality now is not feasible.
There was a problem hiding this comment.
Maybe add a TODO message to follow up on this.
What is this feature?
Introduces the datagrid panel. A new spreadsheet-like view in which users can edit data within grafana and use it in their panels.
Why do we need this feature?
Provides a new way of interacting with a dataset, where users may fine-tune their data before usage. This would prove beneficial to users working a lot with spreadsheets since they will now be able to do their editing within grafana and be more productive.
Who is this feature for?
Users that mostly use spreadsheets and spreadsheet-related datasources to pull their data in grafana.
Which issue(s) does this PR fix?:
Fixes #
Special notes for your reviewer:
Please check that: