-
Notifications
You must be signed in to change notification settings - Fork 88
app/vmui: allow table column reordering #957
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
base: master
Are you sure you want to change the base?
app/vmui: allow table column reordering #957
Conversation
|
@withlin , thank you for the pull request! @Loori-R , @arturminchukov , could you review it? |
- add drag-and-drop ordering in table settings and store order in URL - render table columns in chosen order and restyle drag handle - add TableLogs order unit test Testing: npm run lint:local; npm test Fixes: VictoriaMetrics#714
f5fbc53 to
762f8c6
Compare
Loori-R
left a comment
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.
Thanks for the PR - overall looks good.
Just a couple of minor UI follow-ups (see attached screenshot):
- Please add the column order index (
#1,#2, …) to the drag list as shown in the screenshot, to make the current order easier to scan and verify. - Please limit the height of the reorder list (similar to the checkbox list) and make it scrollable, as shown in the screenshot, to avoid the modal growing too tall when many columns are selected.
|
|
||
| const handleDragOver = (column: string) => (e: DragEvent) => { | ||
| e.preventDefault(); | ||
| if (dragOverColumn === column) return; |
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.
dragOverColumn can be stale here. This check can be removed - setting the same value won’t trigger a re-render.
| setDragOverColumn(null); | ||
| setDraggingColumn(null); |
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.
Cleanup logic is duplicated here - consider calling handleDragEnd() instead of resetting the state inline:
| setDragOverColumn(null); | |
| setDraggingColumn(null); | |
| handleDragEnd(); |
| return displayColumns.reduce<typeof tableColumns>((acc, key) => { | ||
| const column = tableColumnsMap.get(key); | ||
| if (column) acc.push(column); | ||
| return acc; | ||
| }, []); | ||
| }, [tableColumnsMap, displayColumns, tableCompact]); |
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.
Since the column config is derived from the key, tableColumnsMap/reduce (and even columns as a source list) look redundant here. We can build the Table columns directly from displayColumns to keep the requested order and simplify the code.
| return displayColumns.reduce<typeof tableColumns>((acc, key) => { | |
| const column = tableColumnsMap.get(key); | |
| if (column) acc.push(column); | |
| return acc; | |
| }, []); | |
| }, [tableColumnsMap, displayColumns, tableCompact]); | |
| return displayColumns.map((key) => ({ | |
| key: key as keyof Logs, | |
| title: key, | |
| className: getColumnClass(key), | |
| })); | |
| }, [displayColumns, tableCompact]); |
Fixes: #714
Checklist
The following checks are mandatory: