From 66e414b7902470929f084899dc2f2a6f1411b2ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christophe=20No=C3=ABl?= Date: Tue, 7 Dec 2021 21:35:09 +0100 Subject: [PATCH 1/5] Add modal confirmation when deleting file --- src/fileManager/_components/FileManager.js | 24 +++++++++++++-- src/songImporter/_themes.scss | 1 - src/ui/_components/Modal.scss | 35 +++++++++++++++++++++- src/ui/_components/ModalConfirm.js | 28 +++++++++++++++++ src/ui/layout/app/_themes.scss | 5 ++++ 5 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 src/ui/_components/ModalConfirm.js diff --git a/src/fileManager/_components/FileManager.js b/src/fileManager/_components/FileManager.js index 2345d35e..060672d0 100644 --- a/src/fileManager/_components/FileManager.js +++ b/src/fileManager/_components/FileManager.js @@ -1,15 +1,20 @@ import './FileManager.scss'; -import React from 'react'; +import React, { useState } from 'react'; import PropTypes from 'prop-types'; import exportSelectedFileAsText from '../exportSelectedFileAsText'; + +import Modal from "../../ui/_components/Modal"; +import ModalConfirm from "../../ui/_components/ModalConfirm"; import Icon from '../../ui/_components/Icon'; import FileActions from './FileActions'; import FileEntry from './FileEntry'; function FileManager(props) { + const [isDeleting, setIsDeleting ] = useState(false); + const { allTitles, selected, @@ -25,8 +30,22 @@ function FileManager(props) { setEditorMode, } = props; + const cancelDelete = () => setIsDeleting(false); + const confirmDelete = () => { + setIsDeleting(false); + deleteFile(selected); + }; + const confirmDeleteModal = !(isDeleting) + ? null + : + + Are you sure you want to delete? + + ; + return (
+ {confirmDeleteModal}
@@ -37,7 +56,8 @@ function FileManager(props) { createFile(defaultTitle)} - deleteFile={() => deleteFile(selected)} + //deleteFile={() => deleteFile(selected)} + deleteFile={() => setIsDeleting(true)} enableRename={() => enableRename(selected)} startImport={() => startImport()} exportAsText={() => { diff --git a/src/songImporter/_themes.scss b/src/songImporter/_themes.scss index e5bb9b48..55664df0 100644 --- a/src/songImporter/_themes.scss +++ b/src/songImporter/_themes.scss @@ -9,7 +9,6 @@ $themes: ( modal-bg: cv(dark-secondary, dark5), modal-border: cv(dark-foreground, dark20), modal-txt: cv(dark-foreground), - //modal-txtDisabled: cv(dark-foreground, dark20), input-bg: cv(dark-secondary, dark10), input-txt: cv(dark-foreground), inputHeader-txt: cv(dark-foreground, fade), diff --git a/src/ui/_components/Modal.scss b/src/ui/_components/Modal.scss index fb331d67..ff406596 100644 --- a/src/ui/_components/Modal.scss +++ b/src/ui/_components/Modal.scss @@ -1,8 +1,22 @@ @import '../../../scss/abstract'; -@import '../layout/app/themes'; + +$themes: ( + light: (), + dark: ( + modal-bg: cv(dark-secondary, dark5), + modal-border: cv(dark-foreground, dark20), + modal-txt: cv(dark-foreground), + ), +); + .mod-ModalContainer { z-index: $zindex-modal; + height: 100%; + width: 100%; + position: absolute; + top: 0; + left: 0; } .mod-Overlay { @@ -17,3 +31,22 @@ .mod-ContentContainer { } + +.mod-ModalConfirmContainer { + z-index: $zindex-modal; + + position: absolute; + top: 50%; + left: 50%; + transform: translate(-50%, -50%); + width: auto; + height: auto; + + padding: 10px; + + @include themify($themes) { + background-color: themed('modal-bg'); + border: 1px solid themed('modal-border'); + color: themed('modal-txt'); + } +} diff --git a/src/ui/_components/ModalConfirm.js b/src/ui/_components/ModalConfirm.js new file mode 100644 index 00000000..0e203450 --- /dev/null +++ b/src/ui/_components/ModalConfirm.js @@ -0,0 +1,28 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +import Button from "./Button"; + +function ModalConfirm(props) { + const { confirmAction, confirmTitle = 'OK', cancelAction, cancelTitle = 'CANCEL', children } = props; + + return ( +
+
{children}
+
+ + +
+
+ ); +} + +ModalConfirm.propTypes = { + confirmAction: PropTypes.func.isRequired, + confirmTitle: PropTypes.string, + cancelAction: PropTypes.func.isRequired, + cancelTitle: PropTypes.string, + children: PropTypes.element.isRequired, +}; + +export default ModalConfirm; diff --git a/src/ui/layout/app/_themes.scss b/src/ui/layout/app/_themes.scss index 1520de6b..e32d8812 100644 --- a/src/ui/layout/app/_themes.scss +++ b/src/ui/layout/app/_themes.scss @@ -1,6 +1,7 @@ $themes: ( light: (), dark: ( + // leftbar leftBar-txt: cv(dark-foreground), leftBar-bg: cv(dark-background), leftBar-bgHover: cv(dark-background, light10), @@ -9,6 +10,7 @@ $themes: ( leftBar-collapser-bg: cv(dark-primary, light10), leftBar-collapser-bgHover: cv(dark-primary, light20), leftBar-collapser-border: cv(dark-foreground, dark30), + // header header-txt: cv(dark-foreground, fade), header-txtHover: cv(dark-foreground), header-txtActive: cv(dark-foreground), @@ -18,9 +20,11 @@ $themes: ( header-bgActive: cv(dark-background, dark10), header-bgDisabled: cv(dark-background), header-border: cv(dark-background, dark10), + // footer footer-txt: cv(dark-foreground, fade), footer-bg: cv(dark-background), footer-border: cv(dark-secondary, dark20), + // rightbar rightBar-txt: cv(dark-foreground), rightBar-bg: cv(dark-background), rightBar-bgHover: cv(dark-background, light10), @@ -29,5 +33,6 @@ $themes: ( rightBar-collapser-bg: cv(dark-primary, light10), rightBar-collapser-bgHover: cv(dark-primary, light20), rightBar-collapser-border: cv(dark-foreground, dark30), + // ), ); From b9b991a7af7f8433ab36fd8ab3f23eab481e1737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christophe=20No=C3=ABl?= Date: Tue, 7 Dec 2021 21:42:26 +0100 Subject: [PATCH 2/5] fix store migration when store is empty --- src/state/store.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/state/store.js b/src/state/store.js index da9c1aad..87eb79da 100644 --- a/src/state/store.js +++ b/src/state/store.js @@ -18,7 +18,9 @@ export function createStore() { const persistedState = loadState(); // store migrations - delete persistedState.db.options.rendering; // remove old options before the options refactor in v0.9.0 + if (persistedState) { + delete (((persistedState || {}).db || {}).options || {}).rendering; // remove old options before the options refactor in v0.9.0 + } /* Reset all options * / Object.keys(persistedState.db.files.allFiles).forEach((fileId) => { From 4fe8dbff9804b9a4b721b11d73118391d344c522 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christophe=20No=C3=ABl?= Date: Tue, 7 Dec 2021 21:54:57 +0100 Subject: [PATCH 3/5] extract deletion confirm modal in own component --- .../_components/DeleteConfirmModal.js | 42 +++++++++++++++++++ src/fileManager/_components/FileManager.js | 29 ++++--------- src/ui/_components/Modal.scss | 9 ++-- 3 files changed, 57 insertions(+), 23 deletions(-) create mode 100644 src/fileManager/_components/DeleteConfirmModal.js diff --git a/src/fileManager/_components/DeleteConfirmModal.js b/src/fileManager/_components/DeleteConfirmModal.js new file mode 100644 index 00000000..3bbd1462 --- /dev/null +++ b/src/fileManager/_components/DeleteConfirmModal.js @@ -0,0 +1,42 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +import Modal from '../../ui/_components/Modal'; +import ModalConfirm from '../../ui/_components/ModalConfirm'; + +function DeleteConfirmModal(props) { + const { selected, deleteFile, isDeleting, setIsDeleting } = props; + + if (!isDeleting) { + return null; + } + + const cancelDelete = () => setIsDeleting(false); + const confirmDelete = () => { + setIsDeleting(false); + deleteFile(selected); + }; + + return ( + + + Are you sure you want to delete this file? +
+ This action cannot be undone. + + + ); +} + +DeleteConfirmModal.propTypes = { + deleteFile: PropTypes.func.isRequired, + isDeleting: PropTypes.bool.isRequired, + selected: PropTypes.string.isRequired, + setIsDeleting: PropTypes.func.isRequired, +}; + +export default DeleteConfirmModal; diff --git a/src/fileManager/_components/FileManager.js b/src/fileManager/_components/FileManager.js index 060672d0..24fcc641 100644 --- a/src/fileManager/_components/FileManager.js +++ b/src/fileManager/_components/FileManager.js @@ -5,16 +5,14 @@ import PropTypes from 'prop-types'; import exportSelectedFileAsText from '../exportSelectedFileAsText'; - -import Modal from "../../ui/_components/Modal"; -import ModalConfirm from "../../ui/_components/ModalConfirm"; +import DeleteConfirmModal from './DeleteConfirmModal'; import Icon from '../../ui/_components/Icon'; import FileActions from './FileActions'; import FileEntry from './FileEntry'; function FileManager(props) { - const [isDeleting, setIsDeleting ] = useState(false); - + const [isDeleting, setIsDeleting] = useState(false); + const { allTitles, selected, @@ -30,22 +28,14 @@ function FileManager(props) { setEditorMode, } = props; - const cancelDelete = () => setIsDeleting(false); - const confirmDelete = () => { - setIsDeleting(false); - deleteFile(selected); - }; - const confirmDeleteModal = !(isDeleting) - ? null - : - - Are you sure you want to delete? - - ; - return (
- {confirmDeleteModal} +
@@ -56,7 +46,6 @@ function FileManager(props) { createFile(defaultTitle)} - //deleteFile={() => deleteFile(selected)} deleteFile={() => setIsDeleting(true)} enableRename={() => enableRename(selected)} startImport={() => startImport()} diff --git a/src/ui/_components/Modal.scss b/src/ui/_components/Modal.scss index ff406596..ba01bef7 100644 --- a/src/ui/_components/Modal.scss +++ b/src/ui/_components/Modal.scss @@ -9,7 +9,6 @@ $themes: ( ), ); - .mod-ModalContainer { z-index: $zindex-modal; height: 100%; @@ -41,8 +40,8 @@ $themes: ( transform: translate(-50%, -50%); width: auto; height: auto; - - padding: 10px; + + padding: 10px 20px; @include themify($themes) { background-color: themed('modal-bg'); @@ -50,3 +49,7 @@ $themes: ( color: themed('modal-txt'); } } + +.mod-ModalConfirmButtons { + text-align: center; +} From e55af9e56e0b75e77bba9c842ecf54405a23fb26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christophe=20No=C3=ABl?= Date: Tue, 7 Dec 2021 22:31:20 +0100 Subject: [PATCH 4/5] Add unit test for confirm dialog and its usage on file delete --- .../_components/DeleteConfirmModal.js | 21 ++--- src/fileManager/_components/FileActions.js | 12 +-- src/ui/_components/ModalConfirm.js | 41 ++++++-- .../_containers/FileManager.spec.js | 6 ++ .../_components/FileManager.spec.js | 26 ++++- .../unit/ui/_components/ModalConfirm.spec.js | 94 +++++++++++++++++++ 6 files changed, 171 insertions(+), 29 deletions(-) create mode 100644 tests/unit/ui/_components/ModalConfirm.spec.js diff --git a/src/fileManager/_components/DeleteConfirmModal.js b/src/fileManager/_components/DeleteConfirmModal.js index 3bbd1462..16d02060 100644 --- a/src/fileManager/_components/DeleteConfirmModal.js +++ b/src/fileManager/_components/DeleteConfirmModal.js @@ -1,7 +1,6 @@ import React from 'react'; import PropTypes from 'prop-types'; -import Modal from '../../ui/_components/Modal'; import ModalConfirm from '../../ui/_components/ModalConfirm'; function DeleteConfirmModal(props) { @@ -18,17 +17,15 @@ function DeleteConfirmModal(props) { }; return ( - - - Are you sure you want to delete this file? -
- This action cannot be undone. - - + + Are you sure you want to delete this file? +
+ This action cannot be undone. + ); } diff --git a/src/fileManager/_components/FileActions.js b/src/fileManager/_components/FileActions.js index 57b8e74f..ab6d4b3f 100644 --- a/src/fileManager/_components/FileActions.js +++ b/src/fileManager/_components/FileActions.js @@ -39,18 +39,18 @@ function FileActions(props) { action: deleteFile, isDisabled: !selected, }, - { - icon: 'download', - text: 'Export', - action: exportAsText, - isDisabled: !selected, - }, { icon: 'print', text: 'Print', action: printFile, isDisabled: !selected, }, + { + icon: 'download', + text: 'Export', + action: exportAsText, + isDisabled: !selected, + }, ]; return ( diff --git a/src/ui/_components/ModalConfirm.js b/src/ui/_components/ModalConfirm.js index 0e203450..76af89c0 100644 --- a/src/ui/_components/ModalConfirm.js +++ b/src/ui/_components/ModalConfirm.js @@ -1,19 +1,40 @@ import React from 'react'; import PropTypes from 'prop-types'; -import Button from "./Button"; +import Button from './Button'; +import Modal from './Modal'; function ModalConfirm(props) { - const { confirmAction, confirmTitle = 'OK', cancelAction, cancelTitle = 'CANCEL', children } = props; + const { + confirmAction, + confirmTitle = 'OK', + cancelAction, + cancelTitle = 'CANCEL', + children, + } = props; return ( -
-
{children}
-
- - -
-
+ +
+
{children}
+
+ + +
+
+ ); } @@ -22,7 +43,7 @@ ModalConfirm.propTypes = { confirmTitle: PropTypes.string, cancelAction: PropTypes.func.isRequired, cancelTitle: PropTypes.string, - children: PropTypes.element.isRequired, + children: PropTypes.node.isRequired, }; export default ModalConfirm; diff --git a/tests/integration/fileManager/_containers/FileManager.spec.js b/tests/integration/fileManager/_containers/FileManager.spec.js index 411060dc..57b21ff4 100644 --- a/tests/integration/fileManager/_containers/FileManager.spec.js +++ b/tests/integration/fileManager/_containers/FileManager.spec.js @@ -67,6 +67,12 @@ describe('FileManager', () => { fireEvent.click(deleteFileBtn); }); + const confirmBtn = getByText('DELETE'); + + act(() => { + fireEvent.click(confirmBtn); + }); + // Check file has been removed allFiles = fileSelectors.getAllTitles(getState()); expect(allFiles.length).toBe(0); diff --git a/tests/unit/fileManager/_components/FileManager.spec.js b/tests/unit/fileManager/_components/FileManager.spec.js index 561cbda5..4952e076 100644 --- a/tests/unit/fileManager/_components/FileManager.spec.js +++ b/tests/unit/fileManager/_components/FileManager.spec.js @@ -329,7 +329,7 @@ describe('FileManager', () => { }); describe('deleteFile', () => { - test('should call deleteFile() on Delete Action click with selected id', () => { + test('should ask for confirmation before deleting a file', () => { const { getByText } = render( ); @@ -337,8 +337,32 @@ describe('FileManager', () => { fireEvent.click(input); + expect(deleteFile).toHaveBeenCalledTimes(0); + + const deleteConfirmBtn = getByText('DELETE'); + + fireEvent.click(deleteConfirmBtn); + + expect(deleteFile).toHaveBeenCalledTimes(1); expect(deleteFile).toHaveBeenCalledWith(props.allTitles[2].id); }); + + test('should allow to cancel file deletion', () => { + const { getByText } = render( + + ); + const input = getByText('Delete'); + + fireEvent.click(input); + + expect(deleteFile).toHaveBeenCalledTimes(0); + + const deleteConfirmBtn = getByText('CANCEL'); + + fireEvent.click(deleteConfirmBtn); + + expect(deleteFile).toHaveBeenCalledTimes(0); + }); }); describe('import', () => { diff --git a/tests/unit/ui/_components/ModalConfirm.spec.js b/tests/unit/ui/_components/ModalConfirm.spec.js new file mode 100644 index 00000000..af2deb77 --- /dev/null +++ b/tests/unit/ui/_components/ModalConfirm.spec.js @@ -0,0 +1,94 @@ +import React from 'react'; + +import { render, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom/extend-expect'; +import userEvent from '@testing-library/user-event'; + +import ModalConfirm from '../../../../src/ui/_components/ModalConfirm'; + +afterEach(cleanup); + +let props; +const confirmAction = jest.fn(); +const cancelAction = jest.fn(); + +beforeEach(() => { + confirmAction.mockClear(); + cancelAction.mockClear(); + + props = { + confirmAction, + cancelAction, + }; +}); + +describe('ModalConfirm', () => { + const modalContent = 'I am the modal content'; + + test('Should render component given as children', () => { + const modal = render( + +
{modalContent}
+ + ); + const renderedModal = modal.getByText(modalContent); + + expect(renderedModal).toBeInTheDocument(); + }); + + test('Should render OK and CANCEL buttons by default', () => { + const modal = render( + +
{modalContent}
+ + ); + const okBtn = modal.getByText('OK'); + const cancelBtn = modal.getByText('CANCEL'); + + expect(okBtn).toBeInTheDocument(); + expect(cancelBtn).toBeInTheDocument(); + }); + + test('Should all customizing OK and CANCEL buttons', () => { + const modal = render( + +
{modalContent}
+ + ); + const okBtn = modal.getByText('CONFIRM'); + const cancelBtn = modal.getByText('BYEBYE'); + + expect(okBtn).toBeInTheDocument(); + expect(cancelBtn).toBeInTheDocument(); + }); + + test('OK should call confirmAction()', () => { + const modal = render( + +
{modalContent}
+ + ); + const okBtn = modal.getByText('OK'); + + userEvent.click(okBtn); + + expect(confirmAction).toHaveBeenCalledTimes(1); + }); + + test('CANCEL should call cancelAction()', () => { + const modal = render( + +
{modalContent}
+ + ); + const okBtn = modal.getByText('CANCEL'); + + userEvent.click(okBtn); + + expect(cancelAction).toHaveBeenCalledTimes(1); + }); +}); From 7251e2637b2711addc62429c25b1ce2533b68fd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christophe=20No=C3=ABl?= Date: Tue, 7 Dec 2021 23:07:29 +0100 Subject: [PATCH 5/5] fix migration unit tets --- src/state/store.js | 8 ++++---- tests/unit/state/store.spec.js | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/state/store.js b/src/state/store.js index 87eb79da..cc8ea99d 100644 --- a/src/state/store.js +++ b/src/state/store.js @@ -18,17 +18,17 @@ export function createStore() { const persistedState = loadState(); // store migrations - if (persistedState) { - delete (((persistedState || {}).db || {}).options || {}).rendering; // remove old options before the options refactor in v0.9.0 + if (persistedState && persistedState.db && persistedState.db.options) { + delete persistedState.db.options.rendering; // remove old options before the options refactor in v0.9.0 } - + /* Reset all options * / Object.keys(persistedState.db.files.allFiles).forEach((fileId) => { delete persistedState.db.files.allFiles[fileId].options; }); delete persistedState.db.options; /**/ - /* reset song Importer state * / + /* misc * / delete persistedState.songImporter; delete persistedState.fileManager.selected; /**/ diff --git a/tests/unit/state/store.spec.js b/tests/unit/state/store.spec.js index 790c3665..55e394e1 100644 --- a/tests/unit/state/store.spec.js +++ b/tests/unit/state/store.spec.js @@ -53,3 +53,24 @@ describe('localStorage persistence', () => { ); }); }); + +describe.each([ + /* */ + ['non existant store', undefined], + ['empty object', {}], + ['empty db', { db: {} }], + ['empty option', { db: { options: {} } }], + ['with rendering options', { db: { options: { rendering: {} } } }], + /**/ +])('migration: %s', (title, state) => { + test('should remove rendering options', () => { + localStorage.__STORE__.state = JSON.stringify(state); + + reducers.mockImplementation((initialState) => initialState); + + createStore(); + const actualState = getStore().getState(); + + expect(actualState.db.options.rendering).toBeUndefined(); + }); +});