From e876d14b92673ec75fe942eb17195583a067ef25 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Wed, 28 Feb 2024 16:43:41 +0000 Subject: [PATCH 1/3] Ensure unsaved pending changes to rows are applied when changing cell --- .../src/components/grid/stores/rows.js | 51 ++++++++++++++----- .../src/components/grid/stores/ui.js | 10 ++++ .../src/components/grid/stores/validation.js | 22 +++++++- 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/packages/frontend-core/src/components/grid/stores/rows.js b/packages/frontend-core/src/components/grid/stores/rows.js index 34ba77afa2..f4b0e97942 100644 --- a/packages/frontend-core/src/components/grid/stores/rows.js +++ b/packages/frontend-core/src/components/grid/stores/rows.js @@ -328,25 +328,34 @@ export const createActions = context => { } // Patches a row with some changes - const updateRow = async (rowId, changes, options = { save: true }) => { + const updateRow = async ( + rowId, + changes, + options = { save: true, force: false } + ) => { const $rows = get(rows) const $rowLookupMap = get(rowLookupMap) const index = $rowLookupMap[rowId] const row = $rows[index] - if (index == null || !Object.keys(changes || {}).length) { + if (index == null) { + return + } + if (!options?.force && !Object.keys(changes || {}).length) { return } // Abandon if no changes - let same = true - for (let column of Object.keys(changes)) { - if (row[column] !== changes[column]) { - same = false - break + if (!options?.force) { + let same = true + for (let column of Object.keys(changes)) { + if (row[column] !== changes[column]) { + same = false + break + } + } + if (same) { + return } - } - if (same) { - return } // Immediately update state so that the change is reflected @@ -359,7 +368,7 @@ export const createActions = context => { })) // Stop here if we don't want to persist the change - if (!options?.save) { + if (!options?.save && !options?.force) { return } @@ -508,7 +517,14 @@ export const createActions = context => { } export const initialise = context => { - const { rowChangeCache, inProgressChanges, previousFocusedRowId } = context + const { + rowChangeCache, + inProgressChanges, + previousFocusedRowId, + previousFocusedCellId, + rows, + validation, + } = context // Wipe the row change cache when changing row previousFocusedRowId.subscribe(id => { @@ -519,4 +535,15 @@ export const initialise = context => { }) } }) + + // Ensure any unsaved changes are saved when changing cell + previousFocusedCellId.subscribe(id => { + const rowId = id?.split("-")[0] + const hasErrors = validation.actions.rowHasErrors(rowId) + const hasChanges = Object.keys(get(rowChangeCache)[rowId] || {}).length > 0 + const isSavingChanges = get(inProgressChanges)[rowId] + if (rowId && !hasErrors && hasChanges && !isSavingChanges) { + rows.actions.updateRow(rowId, null, { force: true }) + } + }) } diff --git a/packages/frontend-core/src/components/grid/stores/ui.js b/packages/frontend-core/src/components/grid/stores/ui.js index 129d6614e5..da0558bb5b 100644 --- a/packages/frontend-core/src/components/grid/stores/ui.js +++ b/packages/frontend-core/src/components/grid/stores/ui.js @@ -16,6 +16,7 @@ export const createStores = context => { const hoveredRowId = writable(null) const rowHeight = writable(get(props).fixedRowHeight || DefaultRowHeight) const previousFocusedRowId = writable(null) + const previousFocusedCellId = writable(null) const gridFocused = writable(false) const isDragging = writable(false) const buttonColumnWidth = writable(0) @@ -48,6 +49,7 @@ export const createStores = context => { focusedCellAPI, focusedRowId, previousFocusedRowId, + previousFocusedCellId, hoveredRowId, rowHeight, gridFocused, @@ -129,6 +131,7 @@ export const initialise = context => { const { focusedRowId, previousFocusedRowId, + previousFocusedCellId, rows, focusedCellId, selectedRows, @@ -181,6 +184,13 @@ export const initialise = context => { lastFocusedRowId = id }) + // Remember the last focused cell ID so that we can store the previous one + let lastFocusedCellId = null + focusedCellId.subscribe(id => { + previousFocusedCellId.set(lastFocusedCellId) + lastFocusedCellId = id + }) + // Remove hovered row when a cell is selected focusedCellId.subscribe(cell => { if (cell && get(hoveredRowId)) { diff --git a/packages/frontend-core/src/components/grid/stores/validation.js b/packages/frontend-core/src/components/grid/stores/validation.js index 9c3927f9c9..70db076593 100644 --- a/packages/frontend-core/src/components/grid/stores/validation.js +++ b/packages/frontend-core/src/components/grid/stores/validation.js @@ -1,8 +1,23 @@ -import { writable, get } from "svelte/store" +import { writable, get, derived } from "svelte/store" +// Normally we would break out actions into the explicit "createActions" +// function, but for validation all these actions are pure so can go into +// "createStores" instead to make dependency ordering simpler export const createStores = () => { const validation = writable({}) + // Derive which rows have errors so that we can use that info later + const rowErrorMap = derived(validation, $validation => { + let map = {} + Object.entries($validation).forEach(([key, error]) => { + // Extract row ID from all errored cell IDs + if (error) { + map[key.split("-")[0]] = true + } + }) + return map + }) + const setError = (cellId, error) => { if (!cellId) { return @@ -13,11 +28,16 @@ export const createStores = () => { })) } + const rowHasErrors = rowId => { + return get(rowErrorMap)[rowId] + } + return { validation: { ...validation, actions: { setError, + rowHasErrors, }, }, } From acecea570499d26ddd453157d6abedfc86a43dff Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Thu, 29 Feb 2024 10:30:38 +0000 Subject: [PATCH 2/3] Refactor grid row actions to be more explicit and remove extraneous flags --- .../src/components/grid/cells/DataCell.svelte | 4 +- .../grid/overlays/KeyboardManager.svelte | 4 +- .../src/components/grid/stores/rows.js | 100 +++++++++--------- 3 files changed, 54 insertions(+), 54 deletions(-) diff --git a/packages/frontend-core/src/components/grid/cells/DataCell.svelte b/packages/frontend-core/src/components/grid/cells/DataCell.svelte index cdaf28978a..d8cff26b9d 100644 --- a/packages/frontend-core/src/components/grid/cells/DataCell.svelte +++ b/packages/frontend-core/src/components/grid/cells/DataCell.svelte @@ -59,13 +59,13 @@ isReadonly: () => readonly, getType: () => column.schema.type, getValue: () => row[column.name], - setValue: (value, options = { save: true }) => { + setValue: (value, options = { apply: true }) => { validation.actions.setError(cellId, null) updateValue({ rowId: row._id, column: column.name, value, - save: options?.save, + apply: options?.apply, }) }, } diff --git a/packages/frontend-core/src/components/grid/overlays/KeyboardManager.svelte b/packages/frontend-core/src/components/grid/overlays/KeyboardManager.svelte index 8b0a0f0942..5e3a035d89 100644 --- a/packages/frontend-core/src/components/grid/overlays/KeyboardManager.svelte +++ b/packages/frontend-core/src/components/grid/overlays/KeyboardManager.svelte @@ -217,14 +217,14 @@ const type = $focusedCellAPI.getType() if (type === "number" && keyCodeIsNumber(keyCode)) { // Update the value locally but don't save it yet - $focusedCellAPI.setValue(parseInt(key), { save: false }) + $focusedCellAPI.setValue(parseInt(key), { apply: false }) $focusedCellAPI.focus() } else if ( ["string", "barcodeqr", "longform"].includes(type) && (keyCodeIsLetter(keyCode) || keyCodeIsNumber(keyCode)) ) { // Update the value locally but don't save it yet - $focusedCellAPI.setValue(key, { save: false }) + $focusedCellAPI.setValue(key, { apply: false }) $focusedCellAPI.focus() } } diff --git a/packages/frontend-core/src/components/grid/stores/rows.js b/packages/frontend-core/src/components/grid/stores/rows.js index f4b0e97942..c8d27da2e7 100644 --- a/packages/frontend-core/src/components/grid/stores/rows.js +++ b/packages/frontend-core/src/components/grid/stores/rows.js @@ -3,6 +3,7 @@ import { fetchData } from "../../../fetch" import { NewRowID, RowPageSize } from "../lib/constants" import { tick } from "svelte" import { Helpers } from "@budibase/bbui" +import { isValid } from "@budibase/string-templates" export const createStores = () => { const rows = writable([]) @@ -327,38 +328,31 @@ export const createActions = context => { get(fetch)?.getInitialData() } - // Patches a row with some changes - const updateRow = async ( - rowId, - changes, - options = { save: true, force: false } - ) => { + // Checks if a changeset for a row actually mutates the row or not + const changesAreValid = (row, changes) => { + const columns = Object.keys(changes || {}) + if (!row || !columns.length) { + return false + } + + // Ensure there is at least 1 column that creates a difference + return columns.some(column => row[column] !== changes[column]) + } + + // Patches a row with some changes in local state, and returns whether a + // valid pending change was made or not + const stashRowChanges = (rowId, changes) => { const $rows = get(rows) const $rowLookupMap = get(rowLookupMap) const index = $rowLookupMap[rowId] const row = $rows[index] - if (index == null) { - return - } - if (!options?.force && !Object.keys(changes || {}).length) { - return + + // Check this is a valid change + if (!row || !changesAreValid(row, changes)) { + return false } - // Abandon if no changes - if (!options?.force) { - let same = true - for (let column of Object.keys(changes)) { - if (row[column] !== changes[column]) { - same = false - break - } - } - if (same) { - return - } - } - - // Immediately update state so that the change is reflected + // Add change to cache rowChangeCache.update(state => ({ ...state, [rowId]: { @@ -366,26 +360,30 @@ export const createActions = context => { ...changes, }, })) + return true + } - // Stop here if we don't want to persist the change - if (!options?.save && !options?.force) { + // Saves any pending changes to a row + const applyRowChanges = async rowId => { + const $rows = get(rows) + const $rowLookupMap = get(rowLookupMap) + const index = $rowLookupMap[rowId] + const row = $rows[index] + if (row == null) { return } // Save change try { - inProgressChanges.update(state => ({ - ...state, - [rowId]: true, - })) + // Mark as in progress + inProgressChanges.update(state => ({ ...state, [rowId]: true })) // Update row - const saved = await datasource.actions.updateRow({ - ...cleanRow(row), - ...get(rowChangeCache)[rowId], - }) + const changes = get(rowChangeCache)[rowId] + const newRow = { ...cleanRow(row), ...changes } + const saved = await datasource.actions.updateRow(newRow) - // Update state after a successful change + // Update row state after a successful change if (saved?._id) { rows.update(state => { state[index] = saved @@ -395,6 +393,8 @@ export const createActions = context => { // Handle users table edge case await refreshRow(saved.id) } + + // Wipe row change cache now that we've saved the row rowChangeCache.update(state => { delete state[rowId] return state @@ -402,15 +402,17 @@ export const createActions = context => { } catch (error) { handleValidationError(rowId, error) } - inProgressChanges.update(state => ({ - ...state, - [rowId]: false, - })) + + // Mark as completed + inProgressChanges.update(state => ({ ...state, [rowId]: false })) } // Updates a value of a row - const updateValue = async ({ rowId, column, value, save = true }) => { - return await updateRow(rowId, { [column]: value }, { save }) + const updateValue = async ({ rowId, column, value, apply = true }) => { + const success = stashRowChanges(rowId, { [column]: value }) + if (success && apply) { + await applyRowChanges(rowId) + } } // Deletes an array of rows @@ -420,9 +422,7 @@ export const createActions = context => { } // Actually delete rows - rowsToDelete.forEach(row => { - delete row.__idx - }) + rowsToDelete.forEach(row => delete row.__idx) await datasource.actions.deleteRows(rowsToDelete) // Update state @@ -442,7 +442,7 @@ export const createActions = context => { newRow = newRows[i] // Ensure we have a unique _id. - // This means generating one for non DS+, overriting any that may already + // This means generating one for non DS+, overwriting any that may already // exist as we cannot allow duplicates. if (!$isDatasourcePlus) { newRow._id = Helpers.uuid() @@ -503,7 +503,7 @@ export const createActions = context => { duplicateRow, getRow, updateValue, - updateRow, + applyRowChanges, deleteRows, hasRow, loadNextPage, @@ -537,13 +537,13 @@ export const initialise = context => { }) // Ensure any unsaved changes are saved when changing cell - previousFocusedCellId.subscribe(id => { + previousFocusedCellId.subscribe(async id => { const rowId = id?.split("-")[0] const hasErrors = validation.actions.rowHasErrors(rowId) const hasChanges = Object.keys(get(rowChangeCache)[rowId] || {}).length > 0 const isSavingChanges = get(inProgressChanges)[rowId] if (rowId && !hasErrors && hasChanges && !isSavingChanges) { - rows.actions.updateRow(rowId, null, { force: true }) + await rows.actions.applyRowChanges(rowId) } }) } From bc723c7094db65e19eabbaff012f126a5037c571 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Thu, 29 Feb 2024 12:25:21 +0000 Subject: [PATCH 3/3] Lint --- packages/frontend-core/src/components/grid/stores/rows.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/frontend-core/src/components/grid/stores/rows.js b/packages/frontend-core/src/components/grid/stores/rows.js index c8d27da2e7..5dc9413ccd 100644 --- a/packages/frontend-core/src/components/grid/stores/rows.js +++ b/packages/frontend-core/src/components/grid/stores/rows.js @@ -3,7 +3,6 @@ import { fetchData } from "../../../fetch" import { NewRowID, RowPageSize } from "../lib/constants" import { tick } from "svelte" import { Helpers } from "@budibase/bbui" -import { isValid } from "@budibase/string-templates" export const createStores = () => { const rows = writable([])