From 976dfcaab72b98a42da1e778d44eeb18b63e93bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 21 May 2025 19:33:23 -0300 Subject: [PATCH] fix: change InplaceTextEditor style and add optimistic update (#1953) (#2014) * Optimistic update for renaming Components, Collections and Containers * Change the InplaceTextEditor to show the new text until the onSave promise resolves * Change the InplaceTextEditor style to: Always show the rename button --- .../InplaceTextEditor.test.tsx | 58 ++++++- src/generic/inplace-text-editor/index.tsx | 154 ++++++------------ .../collections/CollectionInfoHeader.tsx | 15 +- .../component-info/ComponentInfoHeader.tsx | 19 ++- .../containers/ContainerInfoHeader.tsx | 15 +- .../containers/UnitInfo.test.tsx | 2 +- src/library-authoring/data/apiHooks.ts | 30 +++- .../units/LibraryUnitBlocks.tsx | 18 +- .../units/LibraryUnitPage.test.tsx | 36 ++-- .../units/LibraryUnitPage.tsx | 14 +- 10 files changed, 193 insertions(+), 168 deletions(-) diff --git a/src/generic/inplace-text-editor/InplaceTextEditor.test.tsx b/src/generic/inplace-text-editor/InplaceTextEditor.test.tsx index 8914ad6de..c4dbc4619 100644 --- a/src/generic/inplace-text-editor/InplaceTextEditor.test.tsx +++ b/src/generic/inplace-text-editor/InplaceTextEditor.test.tsx @@ -1,6 +1,11 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { fireEvent, render as baseRender, screen } from '@testing-library/react'; +import { + act, + fireEvent, + render as baseRender, + screen, +} from '@testing-library/react'; import { InplaceTextEditor } from '.'; const mockOnSave = jest.fn(); @@ -24,8 +29,8 @@ describe('', () => { expect(screen.queryByRole('button', { name: /edit/ })).not.toBeInTheDocument(); }); - it('should render the edit button if alwaysShowEditButton is true', () => { - render(); + it('should render the edit button', () => { + render(); expect(screen.getByText('Test text')).toBeInTheDocument(); expect(screen.getByRole('button', { name: /edit/i })).toBeInTheDocument(); @@ -36,7 +41,10 @@ describe('', () => { const title = screen.getByText('Test text'); expect(title).toBeInTheDocument(); - fireEvent.click(title); + + const editButton = screen.getByRole('button', { name: /edit/i }); + expect(editButton).toBeInTheDocument(); + fireEvent.click(editButton); const textBox = screen.getByRole('textbox'); @@ -52,7 +60,10 @@ describe('', () => { const title = screen.getByText('Test text'); expect(title).toBeInTheDocument(); - fireEvent.click(title); + + const editButton = screen.getByRole('button', { name: /edit/i }); + expect(editButton).toBeInTheDocument(); + fireEvent.click(editButton); const textBox = screen.getByRole('textbox'); @@ -62,4 +73,41 @@ describe('', () => { expect(textBox).not.toBeInTheDocument(); expect(mockOnSave).not.toHaveBeenCalled(); }); + + it('should show the new text while processing and roolback in case of error', async () => { + let rejecter: (err: Error) => void; + const longMockOnSave = jest.fn().mockReturnValue( + new Promise((_resolve, reject) => { + rejecter = reject; + }), + ); + render(); + + const text = screen.getByText('Test text'); + expect(text).toBeInTheDocument(); + + const editButton = screen.getByRole('button', { name: /edit/i }); + expect(editButton).toBeInTheDocument(); + fireEvent.click(editButton); + + const textBox = screen.getByRole('textbox'); + + fireEvent.change(textBox, { target: { value: 'New text' } }); + fireEvent.keyDown(textBox, { key: 'Enter', code: 'Enter', charCode: 13 }); + + expect(textBox).not.toBeInTheDocument(); + expect(longMockOnSave).toHaveBeenCalledWith('New text'); + + // Show pending new text + const newText = screen.getByText('New text'); + expect(newText).toBeInTheDocument(); + + await act(async () => { rejecter(new Error('error')); }); + + // Remove pending new text on error + expect(newText).not.toBeInTheDocument(); + + // Show original text + expect(screen.getByText('Test text')).toBeInTheDocument(); + }); }); diff --git a/src/generic/inplace-text-editor/index.tsx b/src/generic/inplace-text-editor/index.tsx index 8caecd550..b1bbe531c 100644 --- a/src/generic/inplace-text-editor/index.tsx +++ b/src/generic/inplace-text-editor/index.tsx @@ -1,14 +1,11 @@ import React, { useCallback, - useEffect, useState, - forwardRef, } from 'react'; import { Form, Icon, IconButton, - OverlayTrigger, Stack, } from '@openedx/paragon'; import { Edit } from '@openedx/paragon/icons'; @@ -16,33 +13,11 @@ import { useIntl } from '@edx/frontend-platform/i18n'; import messages from './messages'; -interface IconWrapperProps { - popper: any; - children: React.ReactNode; - [key: string]: any; -} - -const IconWrapper = forwardRef(({ popper, children, ...props }, ref) => { - useEffect(() => { - // This is a workaround to force the popper to update its position when - // the editor is opened. - // Ref: https://react-bootstrap.netlify.app/docs/components/overlays/#updating-position-dynamically - popper.scheduleUpdate(); - }, [popper, children]); - - return ( -
- {children} -
- ); -}); - interface InplaceTextEditorProps { text: string; - onSave: (newText: string) => void; + onSave: (newText: string) => Promise; readOnly?: boolean; textClassName?: string; - alwaysShowEditButton?: boolean; } export const InplaceTextEditor: React.FC = ({ @@ -50,18 +25,29 @@ export const InplaceTextEditor: React.FC = ({ onSave, readOnly = false, textClassName, - alwaysShowEditButton = false, }) => { const intl = useIntl(); const [inputIsActive, setIsActive] = useState(false); + const [pendingSaveText, setPendingSaveText] = useState(); // state with the new text while updating const handleOnChangeText = useCallback( - (event) => { - const newText = event.target.value; - if (newText && newText !== text) { - onSave(newText); - } + async (event: React.ChangeEvent | React.KeyboardEvent) => { + const inputText = event.currentTarget.value; setIsActive(false); + if (inputText && inputText !== text) { + // NOTE: While using react query for optimistic updates would be the best approach, + // it could not be possible in some cases. For that reason, we use the `pendingSaveText` state + // to show the new text while saving. + setPendingSaveText(inputText); + try { + await onSave(inputText); + } catch { + // don't propagate the exception + } finally { + // reset the pending save text + setPendingSaveText(undefined); + } + } }, [text], ); @@ -78,86 +64,44 @@ export const InplaceTextEditor: React.FC = ({ } }; - if (readOnly) { + // If we have the `pendingSaveText` state it means that we are in the process of saving the new text. + // In that case, we show the new text instead of the original in read-only mode as an optimistic update. + if (readOnly || pendingSaveText) { return ( - {text} + {pendingSaveText || text} ); } - if (alwaysShowEditButton) { - return ( - - {inputIsActive - ? ( - - ) - : ( - - {text} - - )} - - - ); - } - return ( - - - - )} + -
- {inputIsActive - ? ( - - ) - : ( - - {text} - - )} -
- + {inputIsActive + ? ( + + ) + : ( + + {text} + + )} + + ); }; diff --git a/src/library-authoring/collections/CollectionInfoHeader.tsx b/src/library-authoring/collections/CollectionInfoHeader.tsx index 8f476d35e..b885d72da 100644 --- a/src/library-authoring/collections/CollectionInfoHeader.tsx +++ b/src/library-authoring/collections/CollectionInfoHeader.tsx @@ -26,14 +26,16 @@ const CollectionInfoHeader = () => { const updateMutation = useUpdateCollection(libraryId, collectionId); const { showToast } = useContext(ToastContext); - const handleSaveTitle = (newTitle: string) => { - updateMutation.mutateAsync({ - title: newTitle, - }).then(() => { + const handleSaveTitle = async (newTitle: string) => { + try { + await updateMutation.mutateAsync({ + title: newTitle, + }); showToast(intl.formatMessage(messages.updateCollectionSuccessMsg)); - }).catch(() => { + } catch (err) { showToast(intl.formatMessage(messages.updateCollectionErrorMsg)); - }); + throw err; + } }; if (!collection) { @@ -46,7 +48,6 @@ const CollectionInfoHeader = () => { text={collection.title} readOnly={readOnly} textClassName="font-weight-bold m-1.5" - alwaysShowEditButton /> ); }; diff --git a/src/library-authoring/component-info/ComponentInfoHeader.tsx b/src/library-authoring/component-info/ComponentInfoHeader.tsx index 0757c9775..11b325694 100644 --- a/src/library-authoring/component-info/ComponentInfoHeader.tsx +++ b/src/library-authoring/component-info/ComponentInfoHeader.tsx @@ -26,16 +26,18 @@ const ComponentInfoHeader = () => { const updateMutation = useUpdateXBlockFields(usageKey); const { showToast } = useContext(ToastContext); - const handleSaveDisplayName = (newDisplayName: string) => { - updateMutation.mutateAsync({ - metadata: { - display_name: newDisplayName, - }, - }).then(() => { + const handleSaveDisplayName = async (newDisplayName: string) => { + try { + await updateMutation.mutateAsync({ + metadata: { + display_name: newDisplayName, + }, + }); showToast(intl.formatMessage(messages.updateComponentSuccessMsg)); - }).catch(() => { + } catch (err) { showToast(intl.formatMessage(messages.updateComponentErrorMsg)); - }); + throw err; + } }; if (!xblockFields) { @@ -48,7 +50,6 @@ const ComponentInfoHeader = () => { text={xblockFields?.displayName} readOnly={readOnly} textClassName="font-weight-bold m-1.5" - alwaysShowEditButton /> ); }; diff --git a/src/library-authoring/containers/ContainerInfoHeader.tsx b/src/library-authoring/containers/ContainerInfoHeader.tsx index 39d590db6..65357c0f1 100644 --- a/src/library-authoring/containers/ContainerInfoHeader.tsx +++ b/src/library-authoring/containers/ContainerInfoHeader.tsx @@ -25,14 +25,16 @@ const ContainerInfoHeader = () => { const updateMutation = useUpdateContainer(containerId); const { showToast } = useContext(ToastContext); - const handleSaveDisplayName = (newDisplayName: string) => { - updateMutation.mutateAsync({ - displayName: newDisplayName, - }).then(() => { + const handleSaveDisplayName = async (newDisplayName: string) => { + try { + await updateMutation.mutateAsync({ + displayName: newDisplayName, + }); showToast(intl.formatMessage(messages.updateContainerSuccessMsg)); - }).catch(() => { + } catch (err) { showToast(intl.formatMessage(messages.updateContainerErrorMsg)); - }); + throw err; + } }; if (!container) { @@ -45,7 +47,6 @@ const ContainerInfoHeader = () => { text={container.displayName} readOnly={readOnly} textClassName="font-weight-bold m-1.5" - alwaysShowEditButton /> ); }; diff --git a/src/library-authoring/containers/UnitInfo.test.tsx b/src/library-authoring/containers/UnitInfo.test.tsx index c44d19604..d6b019d4e 100644 --- a/src/library-authoring/containers/UnitInfo.test.tsx +++ b/src/library-authoring/containers/UnitInfo.test.tsx @@ -106,6 +106,6 @@ describe('', () => { it('show only published content', async () => { render(true); expect(await screen.findByTestId('unit-info-menu-toggle')).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /text block published 1/i })).toBeInTheDocument(); + expect(screen.getByText(/text block published 1/i)).toBeInTheDocument(); }); }); diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index 01dd97c3e..65b9445ad 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -472,15 +472,28 @@ export const useCollection = (libraryId: string, collectionId: string) => ( */ export const useUpdateCollection = (libraryId: string, collectionId: string) => { const queryClient = useQueryClient(); + const collectionQueryKey = libraryAuthoringQueryKeys.collection(libraryId, collectionId); return useMutation({ mutationFn: (data: api.UpdateCollectionComponentsRequest) => ( api.updateCollectionMetadata(libraryId, collectionId, data) ), + onMutate: (data) => { + const previousData = queryClient.getQueryData(collectionQueryKey) as api.CollectionMetadata; + queryClient.setQueryData(collectionQueryKey, { + ...previousData, + ...data, + }); + + return { previousData }; + }, + onError: (_err, _data, context) => { + queryClient.setQueryData(collectionQueryKey, context?.previousData); + }, onSettled: () => { // NOTE: We invalidate the library query here because we need to update the library's // collection list. queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, libraryId) }); - queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.collection(libraryId, collectionId) }); + queryClient.invalidateQueries({ queryKey: collectionQueryKey }); }, }); }; @@ -598,13 +611,26 @@ export const useContainer = (containerId?: string) => ( export const useUpdateContainer = (containerId: string) => { const libraryId = getLibraryId(containerId); const queryClient = useQueryClient(); + const containerQueryKey = libraryAuthoringQueryKeys.container(containerId); return useMutation({ mutationFn: (data: api.UpdateContainerDataRequest) => api.updateContainerMetadata(containerId, data), + onMutate: (data) => { + const previousData = queryClient.getQueryData(containerQueryKey) as api.CollectionMetadata; + queryClient.setQueryData(containerQueryKey, { + ...previousData, + ...data, + }); + + return { previousData }; + }, + onError: (_err, _data, context) => { + queryClient.setQueryData(containerQueryKey, context?.previousData); + }, onSettled: () => { // NOTE: We invalidate the library query here because we need to update the library's // container list. queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, libraryId) }); - queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.container(containerId) }); + queryClient.invalidateQueries({ queryKey: containerQueryKey }); }, }); }; diff --git a/src/library-authoring/units/LibraryUnitBlocks.tsx b/src/library-authoring/units/LibraryUnitBlocks.tsx index ef7d62e1a..8486e275b 100644 --- a/src/library-authoring/units/LibraryUnitBlocks.tsx +++ b/src/library-authoring/units/LibraryUnitBlocks.tsx @@ -63,16 +63,18 @@ const BlockHeader = ({ block }: ComponentBlockProps) => { const updateMutation = useUpdateXBlockFields(block.originalId); - const handleSaveDisplayName = (newDisplayName: string) => { - updateMutation.mutateAsync({ - metadata: { - display_name: newDisplayName, - }, - }).then(() => { + const handleSaveDisplayName = async (newDisplayName: string) => { + try { + await updateMutation.mutateAsync({ + metadata: { + display_name: newDisplayName, + }, + }); showToast(intl.formatMessage(messages.updateComponentSuccessMsg)); - }).catch(() => { + } catch (err) { showToast(intl.formatMessage(messages.updateComponentErrorMsg)); - }); + throw err; + } }; /* istanbul ignore next */ diff --git a/src/library-authoring/units/LibraryUnitPage.test.tsx b/src/library-authoring/units/LibraryUnitPage.test.tsx index 6d4ea7e31..18671ca56 100644 --- a/src/library-authoring/units/LibraryUnitPage.test.tsx +++ b/src/library-authoring/units/LibraryUnitPage.test.tsx @@ -106,12 +106,12 @@ describe('', () => { it('can rename unit', async () => { renderLibraryUnitPage(); expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument(); - // Unit title - const unitTitle = screen.getAllByRole( + + const editUnitTitleButton = screen.getAllByRole( 'button', - { name: mockGetContainerMetadata.containerData.displayName }, - )[0]; - fireEvent.click(unitTitle); + { name: /edit/i }, + )[0]; // 0 is the Unit Title, 1 is the first component on the list + fireEvent.click(editUnitTitleButton); const url = getLibraryContainerApiUrl(mockGetContainerMetadata.containerId); axiosMock.onPatch(url).reply(200); @@ -137,12 +137,12 @@ describe('', () => { it('show error if renaming unit fails', async () => { renderLibraryUnitPage(); expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument(); - // Unit title - const unitTitle = screen.getAllByRole( + + const editUnitTitleButton = screen.getAllByRole( 'button', - { name: mockGetContainerMetadata.containerData.displayName }, - )[0]; - fireEvent.click(unitTitle); + { name: /edit/i }, + )[0]; // 0 is the Unit Title, 1 is the first component on the list + fireEvent.click(editUnitTitleButton); const url = getLibraryContainerApiUrl(mockGetContainerMetadata.containerId); axiosMock.onPatch(url).reply(400); @@ -210,11 +210,11 @@ describe('', () => { // Wait loading of the component await screen.findByText('text block 0'); - const componentTitle = screen.getAllByRole( + const editButton = screen.getAllByRole( 'button', - { name: 'text block 0' }, - )[0]; - fireEvent.click(componentTitle); + { name: /edit/i }, + )[1]; // 0 is the Unit Title, 1 is the first component on the list + fireEvent.click(editButton); await waitFor(() => { expect(screen.getByRole('textbox', { name: /text input/i })).toBeInTheDocument(); @@ -244,11 +244,11 @@ describe('', () => { // Wait loading of the component await screen.findByText('text block 0'); - const componentTitle = screen.getAllByRole( + const editButton = screen.getAllByRole( 'button', - { name: 'text block 0' }, - )[0]; - fireEvent.click(componentTitle); + { name: /edit/i }, + )[1]; // 0 is the Unit Title, 1 is the first component on the list + fireEvent.click(editButton); await waitFor(() => { expect(screen.getByRole('textbox', { name: /text input/i })).toBeInTheDocument(); diff --git a/src/library-authoring/units/LibraryUnitPage.tsx b/src/library-authoring/units/LibraryUnitPage.tsx index a103a9896..c8d337314 100644 --- a/src/library-authoring/units/LibraryUnitPage.tsx +++ b/src/library-authoring/units/LibraryUnitPage.tsx @@ -41,14 +41,16 @@ const EditableTitle = ({ unitId }: EditableTitleProps) => { const updateMutation = useUpdateContainer(unitId); const { showToast } = useContext(ToastContext); - const handleSaveDisplayName = (newDisplayName: string) => { - updateMutation.mutateAsync({ - displayName: newDisplayName, - }).then(() => { + const handleSaveDisplayName = async (newDisplayName: string) => { + try { + await updateMutation.mutateAsync({ + displayName: newDisplayName, + }); showToast(intl.formatMessage(messages.updateContainerSuccessMsg)); - }).catch(() => { + } catch (err) { showToast(intl.formatMessage(messages.updateContainerErrorMsg)); - }); + throw err; + } }; // istanbul ignore if: this should never happen