From 0f9ca0ede3a3db7375f3d4e9a9043bd6b5f9a016 Mon Sep 17 00:00:00 2001 From: matttdawson <89495499+matttdawson@users.noreply.github.com> Date: Wed, 9 Nov 2022 15:33:39 +1300 Subject: [PATCH] fix: post sort rows hook doesnt sort on swap * fix: postSortRowsHook doesnt sort on swap * fix: action button wont spin if not passed a promise * Fix grid height in popout * fix: error on update of destroyed grid * feat: add aria label to action button, fix post sort rows exception on zombie grid * feat: add level to ActionButton * fix: centering spinner --- package.json | 3 +- src/components/PostSortRowsHook.ts | 103 +++++++++++++----- .../GridFormSubComponentTextInput.tsx | 90 +++++++-------- src/contexts/GridContext.tsx | 6 +- src/contexts/GridContextProvider.tsx | 7 ++ src/lui/ActionButton.tsx | 36 +++--- .../components/ActionButton.stories.tsx | 7 +- 7 files changed, 166 insertions(+), 86 deletions(-) diff --git a/package.json b/package.json index a6813762..445b1a98 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "@linzjs/step-ag-grid", "repository": "github:linz/step-ag-grid.git", "license": "MIT", - "version": "2.4.1", + "version": "2.4.2", "keywords": [ "aggrid", "ag-grid", @@ -46,6 +46,7 @@ }, "scripts": { "build": "run-s clean stylelint lint css bundle", + "yalc": "run-s clean css bundle && yalc publish", "clean": "rimraf dist && mkdirp ./dist", "bundle": "rollup -c", "stylelint": "stylelint src/**/*.scss src/**/*.css --fix", diff --git a/src/components/PostSortRowsHook.ts b/src/components/PostSortRowsHook.ts index 5605a3f0..94f9dfbe 100644 --- a/src/components/PostSortRowsHook.ts +++ b/src/components/PostSortRowsHook.ts @@ -1,6 +1,9 @@ -import { useCallback, useRef } from "react"; +import { useCallback, useContext, useRef } from "react"; import { PostSortRowsParams } from "ag-grid-community/dist/lib/entities/iCallbackParams"; import { ColumnState } from "ag-grid-community/dist/lib/columns/columnModel"; +import { isEmpty } from "lodash-es"; +import { RowNode } from "ag-grid-community"; +import { GridContext } from "../contexts/GridContext"; interface PostSortRowsHookProps { setStaleGrid: (stale: boolean) => void; @@ -12,6 +15,8 @@ interface PostSortRowsHookProps { * Handles stale sort, when you edit a row but don't want to re-sort. */ export const usePostSortRowsHook = ({ setStaleGrid }: PostSortRowsHookProps) => { + const { redrawRows } = useContext(GridContext); + // On first run we need to init the first backed up sort order const initialised = useRef(false); @@ -19,7 +24,7 @@ export const usePostSortRowsHook = ({ setStaleGrid }: PostSortRowsHookProps) => const lastSortOrderHash = useRef(""); // Used to detect if sort order was the same, has the direction changed - const previousRowSortIndexRef = useRef>({}); + const previousRowSortIndexRef = useRef>({}); // stale sort is when there's a sort and user edits a row // this applies a class to the div wrapping the grid which in turn adds a * beside the sort arrow @@ -33,14 +38,21 @@ export const usePostSortRowsHook = ({ setStaleGrid }: PostSortRowsHookProps) => return useCallback( ({ api, columnApi, nodes }: PostSortRowsParams) => { + // Grid is destroyed + if (!api || !columnApi) return; + const previousRowSortIndex = previousRowSortIndexRef.current; + const hashNode = (node: RowNode | undefined) => { + return node ? JSON.stringify(node.data) : ""; + }; + const copyCurrentSortSettings = (): ColumnState[] => columnApi.getColumnState().map((row) => ({ colId: row.colId, sort: row.sort, sortIndex: row.sortIndex })); const backupSortOrder = () => { for (const x in previousRowSortIndex) delete previousRowSortIndex[x]; - nodes.forEach((row, index) => (previousRowSortIndex[row.data.id] = index)); + nodes.forEach((node, index) => (previousRowSortIndex[`${node.data.id}`] = { index, hash: hashNode(node) })); }; // Check if column is the first sorted column. Note: column is preconfigured to sort its sortIndex is null not 1 @@ -51,22 +63,13 @@ export const usePostSortRowsHook = ({ setStaleGrid }: PostSortRowsHookProps) => const restorePreviousSortColumnState = () => columnApi.applyColumnState({ state: previousColSort.current }); - const hasNewRows = () => nodes.some((row) => previousRowSortIndex[row.data.id] == null); - - const sortIsStale = () => { - // If there are new rows we want to them to be at the bottom of the grid, so we treat it as sort not stale - if (hasNewRows()) return false; - - // Otherwise check if the stored sort index matches the new sort index - return nodes.some((node, index) => previousRowSortIndex[node.data.id] != index); - }; - - const sortNodesByPreviousSort = () => + const sortNodesByPreviousSort = () => { nodes.sort( (a, b) => - (previousRowSortIndex[a.data.id] ?? Number.MAX_SAFE_INTEGER) - - (previousRowSortIndex[b.data.id] ?? Number.MAX_SAFE_INTEGER), + (previousRowSortIndex[`${a.data.id}`]?.index ?? Number.MAX_SAFE_INTEGER) - + (previousRowSortIndex[`${b.data.id}`]?.index ?? Number.MAX_SAFE_INTEGER), ); + }; // On first load copy the current sort if (!initialised.current) { @@ -83,6 +86,10 @@ export const usePostSortRowsHook = ({ setStaleGrid }: PostSortRowsHookProps) => sortOrderChanged = true; } + if (isEmpty(previousRowSortIndex)) { + backupSortOrder(); + } + if (sortOrderChanged) { const thisFirstCol = copyCurrentSortSettings().find(isFirstSortColumn); const previousFirstCol = previousColSort.current.find(isFirstSortColumn); @@ -115,23 +122,69 @@ export const usePostSortRowsHook = ({ setStaleGrid }: PostSortRowsHookProps) => lastSortOrderHash.current = newSortOrder; } } else { - if (sortIsStale()) { + let firstChangedNodeIndex = -1; + let lastNewNode: RowNode | undefined = undefined; + let changedRowCount = 0; + let newRowCount = 0; + let index = 0; + for (const node of nodes) { + const psr = previousRowSortIndex[`${node.data.id}`]; + if (psr) { + if (psr.hash != hashNode(node)) { + if (firstChangedNodeIndex === -1) firstChangedNodeIndex = index; + changedRowCount++; + } + } else { + lastNewNode = node; + newRowCount++; + } + index++; + } + + let wasStale = false; + if (changedRowCount === 0 && newRowCount === 1) { + // insert new row at end + const newIndex = index - 1; + previousRowSortIndex[`${lastNewNode?.data.id}`] = { index: newIndex, hash: hashNode(lastNewNode) }; + wasStale = true; + } else if (changedRowCount === 2 && newRowCount === 0) { + // This must be a swap rows + backupSortOrder(); + wasStale = false; + } else if (changedRowCount > 1 && newRowCount === 1) { + // This must be a insert so, insert new row near the row that changed + previousRowSortIndex[`${lastNewNode?.data.id}`] = { + index: firstChangedNodeIndex + 0.5, + hash: hashNode(lastNewNode), + }; + wasStale = true; + // For some reason AgGrid mis-positions the inserted row. + lastNewNode && redrawRows(); + } else if (changedRowCount == 1 && newRowCount === 0) { + // User edited one row so, do nothing, retain sort + wasStale = true; + } else if (changedRowCount !== 0 || newRowCount != 0) { + // too many rows changed, resort + backupSortOrder(); + } + + if (wasStale) { + // Check if the sort order the aggrid passed matches our stale sort order + const stillStale = + Object.keys(previousRowSortIndex).length != nodes.length || + nodes.some((node, index) => previousRowSortIndex[`${node.data.id}`]?.index !== index); + // If we haven't already processed a stale sort then... - if (!sortWasStale.current) { + if (stillStale && !sortWasStale.current) { // backup sort state, so we can restore it when sort is clicked on a stale column previousColSort.current = copyCurrentSortSettings(); - backupSortOrder(); sortWasStale.current = true; setStaleGrid(true); } - - sortNodesByPreviousSort(); } - // secondary sort backup as there may be new nodes that didn't have their sort registered - // which would cause two new rows to sort out of sequence - backupSortOrder(); + sortNodesByPreviousSort(); } }, - [setStaleGrid], + [redrawRows, setStaleGrid], ); }; diff --git a/src/components/gridForm/GridFormSubComponentTextInput.tsx b/src/components/gridForm/GridFormSubComponentTextInput.tsx index 987ae407..5354497f 100644 --- a/src/components/gridForm/GridFormSubComponentTextInput.tsx +++ b/src/components/gridForm/GridFormSubComponentTextInput.tsx @@ -1,45 +1,45 @@ -import { useState } from "react"; -import { TextInputFormatted } from "../../lui/TextInputFormatted"; - -export interface GridFormSubComponentTextInput { - setValue: (value: string) => void; - keyDown: (key: string) => void; - placeholder?: string; - className?: string; -} - -export const GridFormSubComponentTextInput = ({ - keyDown, - placeholder, - setValue, - className, -}: GridFormSubComponentTextInput) => { - const placeholderText = placeholder || "Other..."; - const inputClass = className || ""; - const [inputValue, setInputValue] = useState(""); - return ( - <> - { - const value = e.target.value; - setValue(value); - setInputValue(value); - }} - inputProps={{ - onKeyDown: (k: any) => keyDown(k.key), - placeholder: placeholderText, - onMouseEnter: (e) => { - if (document.activeElement != e.currentTarget) { - e.currentTarget.focus(); - } - }, - style: { - width: "100%", - }, - }} - /> - - ); -}; +import { useState } from "react"; +import { TextInputFormatted } from "../../lui/TextInputFormatted"; + +export interface GridFormSubComponentTextInput { + setValue: (value: string) => void; + keyDown: (key: string) => void; + placeholder?: string; + className?: string; +} + +export const GridFormSubComponentTextInput = ({ + keyDown, + placeholder, + setValue, + className, +}: GridFormSubComponentTextInput) => { + const placeholderText = placeholder || "Other..."; + const inputClass = className || ""; + const [inputValue, setInputValue] = useState(""); + return ( + <> + { + const value = e.target.value; + setValue(value); + setInputValue(value); + }} + inputProps={{ + onKeyDown: (k: any) => keyDown(k.key), + placeholder: placeholderText, + onMouseEnter: (e) => { + if (document.activeElement != e.currentTarget) { + e.currentTarget.focus(); + } + }, + style: { + width: "100%", + }, + }} + /> + + ); +}; diff --git a/src/contexts/GridContext.tsx b/src/contexts/GridContext.tsx index 9bf53312..78962a16 100644 --- a/src/contexts/GridContext.tsx +++ b/src/contexts/GridContext.tsx @@ -1,5 +1,5 @@ import { createContext } from "react"; -import { GridApi } from "ag-grid-community"; +import { GridApi, RowNode } from "ag-grid-community"; import { GridBaseRow } from "../components/Grid"; export interface GridContextType { @@ -24,6 +24,7 @@ export interface GridContextType { fnUpdate: (selectedRows: any[]) => Promise, setSaving?: (saving: boolean) => void, ) => Promise; + redrawRows: (rowNodes?: RowNode[]) => void; } export const GridContext = createContext({ @@ -83,4 +84,7 @@ export const GridContext = createContext({ console.error("no context provider for modifyUpdating"); return false; }, + redrawRows: () => { + console.error("no context provider for redrawRows"); + }, }); diff --git a/src/contexts/GridContextProvider.tsx b/src/contexts/GridContextProvider.tsx index dad93405..b2c8113d 100644 --- a/src/contexts/GridContextProvider.tsx +++ b/src/contexts/GridContextProvider.tsx @@ -273,6 +273,12 @@ export const GridContextProvider = (props: GridContextProps): ReactElement => { }); }; + const redrawRows = (rowNodes?: RowNode[]) => { + gridApiOp((gridApi) => { + gridApi.redrawRows(rowNodes ? { rowNodes } : undefined); + }); + }; + return ( { sizeColumnsToFit, stopEditing, updatingCells, + redrawRows, }} > {props.children} diff --git a/src/lui/ActionButton.tsx b/src/lui/ActionButton.tsx index 0061c968..3328001f 100644 --- a/src/lui/ActionButton.tsx +++ b/src/lui/ActionButton.tsx @@ -6,18 +6,21 @@ import { LuiButton, LuiIcon, LuiMiniSpinner } from "@linzjs/lui"; import { IconName } from "@linzjs/lui/dist/components/LuiIcon/LuiIcon"; import { usePrevious } from "./reactUtils"; import { useStateDeferred } from "./stateDeferredHook"; +import { LuiButtonProps } from "@linzjs/lui/dist/components/LuiButton/LuiButton"; export interface ActionButtonProps { icon: IconName; - name: string; + name?: string; + "aria-label"?: string; inProgressName?: string; title?: string; dataTestId?: string; size?: "xs" | "sm" | "md" | "lg" | "xl" | "ns"; className?: string; - onAction: () => Promise; + onAction: () => Promise | void; // Used for external code to get access to whether action is in progress externalSetInProgress?: () => void; + level?: LuiButtonProps["level"]; } // Kept this less than one second, so I don't have issues with waitFor as it defaults to 1s @@ -33,6 +36,8 @@ export const ActionButton = ({ onAction, externalSetInProgress, size = "sm", + level = "tertiary", + "aria-label": ariaLabel, }: ActionButtonProps): JSX.Element => { const [inProgress, setInProgress] = useState(false); const lastInProgress = usePrevious(inProgress ?? false); @@ -47,17 +52,22 @@ export const ActionButton = ({ { - setInProgress(true); - externalSetInProgress && setInProgress(true); - await onAction(); - externalSetInProgress && setInProgress(false); - setInProgress(false); + const promise = onAction(); + const isPromise = typeof promise !== "undefined"; + if (isPromise) { + setInProgress(true); + externalSetInProgress && setInProgress(true); + if (isPromise) await promise; + externalSetInProgress && setInProgress(false); + setInProgress(false); + } }} disabled={localInProgress} > @@ -66,13 +76,13 @@ export const ActionButton = ({ size={16} divProps={{ "data-testid": "loading-spinner", - style: { padding: 0, margin: 0, paddingRight: 8 }, + style: { margin: 0, paddingRight: 5, paddingLeft: 3, paddingBottom: 0, paddingTop: 0 }, role: "status", - ["aria-label"]: "Loading", + "aria-label": "Loading", }} /> ) : ( - + )} {(localInProgress ? inProgressName : name) ?? name} diff --git a/src/stories/components/ActionButton.stories.tsx b/src/stories/components/ActionButton.stories.tsx index e917d7f8..2a743b87 100644 --- a/src/stories/components/ActionButton.stories.tsx +++ b/src/stories/components/ActionButton.stories.tsx @@ -16,7 +16,12 @@ const ActionButtonTemplate: ComponentStory = () => { const performAction = useCallback(async () => { await wait(1000); }, []); - return ; + return ( + <> + + + + ); }; export const ActionButtonSimple = ActionButtonTemplate.bind({});