Skip to content

dbeaver/dbeaver-vscode#83 refactor: data grid #3265

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

Merged
merged 21 commits into from
Feb 25, 2025
Merged

dbeaver/dbeaver-vscode#83 refactor: data grid #3265

merged 21 commits into from
Feb 25, 2025

Conversation

Wroud
Copy link
Member

@Wroud Wroud commented Feb 13, 2025

No description provided.

@Wroud Wroud self-assigned this Feb 13, 2025
sergeyteleshev
sergeyteleshev previously approved these changes Feb 14, 2025
Comment on lines +35 to +46
function getCell(rowIdx: number, colIdx: number) {
switch (colIdx) {
case 0:
return trace![rowIdx]?.name ?? '';
case 1:
return trace![rowIdx]?.value ?? '';
case 2:
return trace![rowIdx]?.description ?? '';
}

return '';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very unusual way of setting the columns. Can we rearrange the columns order in grid? Will it work?

@@ -125,28 +65,70 @@ export const Table = observer<TableProps>(function Table({ objects, hasNextPage,
return null;
}

function getCell(rowIdx: number, colIdx: number) {
colIdx--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we move this mutation to the DataGrid props
so getCell={(r, c) => getCell(r, c--)}?

Comment on lines +97 to +98
function getHeaderText(colIdx: number) {
colIdx--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and same here: remove mutation from the getter

Comment on lines +215 to +264
// const colIdx = tableData.getColumnIndexFromColumnKey(cell.column);
// const rowIdx = tableData.getRowIndexFromKey(cell.row);
// const editingState = tableData.editor.getElementState(cell);

// switch (event.nativeEvent.code) {
// case 'Delete': {
// const filteredRows = activeRows.filter(cell => tableData.editor.getElementState(cell) !== DatabaseEditChangeType.delete);

// if (filteredRows.length > 0) {
// const editor = tableData.editor;
// const firstRow = filteredRows[0]!;
// const editingState = tableData.editor.getElementState(firstRow);

// editor.delete(...filteredRows);

// if (editingState === DatabaseEditChangeType.add) {
// if (rowIdx - 1 > 0) {
// handlers.selectCell({ colIdx, rowIdx: rowIdx - 1 });
// }
// } else {
// if (rowIdx + 1 < tableData.rows.length) {
// handlers.selectCell({ colIdx, rowIdx: rowIdx + 1 });
// }
// }
// }

// return;
// }
// case 'KeyV': {
// if (editingState === DatabaseEditChangeType.delete) {
// return;
// }

// if (event.ctrlKey || event.metaKey) {
// if (!clipboardService.clipboardAvailable || clipboardService.state === 'denied' || tableData.isCellReadonly(cell)) {
// return;
// }

// clipboardService
// .read()
// .then(value => tableData.editor.set(cell, value))
// .catch();
// return;
// }
// }
// }

// if (editingState === DatabaseEditChangeType.delete) {
// return;
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do we do with this feature?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to test and if all works we will just remove this, if not we will return this code back (I think I can just remove it for now and revert if needed)

@SychevAndrey SychevAndrey self-requested a review February 19, 2025 16:56
Comment on lines +30 to +46
const trace = state.trace;

const columnsCount = useCreateGridReactiveValue(() => 3, null, []);
const rowCount = useCreateGridReactiveValue(
() => trace?.length || 0,
onValueChange => reaction(() => trace?.length || 0, onValueChange),
[trace],
);

function getCell(rowIdx: number, colIdx: number) {
switch (colIdx) {
case 0:
return trace![rowIdx]?.name ?? '';
case 1:
return trace![rowIdx]?.value ?? '';
case 2:
return trace![rowIdx]?.description ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this trace variable and why do we access it with trace? and trace! at the same time?

@Wroud Wroud merged commit 080317d into devel Feb 25, 2025
5 checks passed
@Wroud Wroud deleted the refactor/grid branch February 25, 2025 17:04
kkapper pushed a commit to PlaidCloud/cloudbeaver that referenced this pull request Apr 18, 2025
* dbeaver/dbeaver-vscode#83 refactor: data grid

* chore: try to fix build scripts

* chore: commit artifacts

* chore: update yarn.lock

* fix: adjust grid auto-sizing

* chore: fix prettier

* chore: add documentation about typescript crashes

* fix: cell menu

* fix: grid inline editor styles

* fix: cell menu styles

* fix: cell items alignment

* fix: data sync

* feat: support reactive values in grid

* chore: try to fix strange build bug

* chore: restore original file name

* fix: add specific to inline editor styles

* fix: remove delete row shortcut (not working)

* chore: enable common cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants