From 59f6f34083efb0ae07c075e58d96add91a1855c6 Mon Sep 17 00:00:00 2001 From: Alf Eaton <75253002+aeaton-overleaf@users.noreply.github.com> Date: Fri, 5 Mar 2021 13:00:28 +0000 Subject: [PATCH] Merge pull request #3710 from overleaf/ae-refactor-hotkeys-modal Refactor "HotKeys" modal GitOrigin-RevId: 1df86322bac229bb04092e872300e5f1ee4cbddc --- .../app/views/project/editor/left-menu.pug | 1 + .../components/hotkeys-modal-content.js | 177 ----------------- .../hotkeys-modal/components/hotkeys-modal.js | 185 ++++++++++++++++-- .../controllers/hotkeys-modal-controller.js | 9 +- .../frontend/stories/hotkeys-modal.stories.js | 27 +-- .../components/hotkeys-modal.test.js | 51 ++--- 6 files changed, 220 insertions(+), 230 deletions(-) delete mode 100644 services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal-content.js diff --git a/services/web/app/views/project/editor/left-menu.pug b/services/web/app/views/project/editor/left-menu.pug index 7a332fc4a9..6384809ee7 100644 --- a/services/web/app/views/project/editor/left-menu.pug +++ b/services/web/app/views/project/editor/left-menu.pug @@ -225,6 +225,7 @@ aside#left-menu.full-size( handle-hide="handleHide" show="show" track-changes-visible="trackChangesVisible" + is-mac="isMac" ) if showSupport li diff --git a/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal-content.js b/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal-content.js deleted file mode 100644 index e02c21b135..0000000000 --- a/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal-content.js +++ /dev/null @@ -1,177 +0,0 @@ -import React from 'react' -import { Button, Modal, Row, Col } from 'react-bootstrap' -import PropTypes from 'prop-types' -import { Trans, useTranslation } from 'react-i18next' - -function HotkeysModalContent({ - handleHide, - isMac = false, - trackChangesVisible = false -}) { - const { t } = useTranslation() - - const ctrl = isMac ? 'Cmd' : 'Ctrl' - - return ( - <> - - {t('hotkeys')} - - - -

{t('common')}

- - - - - - - - - - - - - - -

{t('navigation')}

- - - - - - - - - - - - - -

{t('editing')}

- - - - - - - - - - - - - - - - - - - - -

{t('autocomplete')}

- - - - - - - - - - - - - -

- }} - /> -

- - - - - - - - {trackChangesVisible && ( - <> -

{t('review')}

- - - - - - - - - - - - - - )} - - - - - - - ) -} - -HotkeysModalContent.propTypes = { - isMac: PropTypes.bool, - handleHide: PropTypes.func.isRequired, - trackChangesVisible: PropTypes.bool -} - -function Hotkey({ combination, description }) { - return ( -
- {combination} - {description} -
- ) -} -Hotkey.propTypes = { - combination: PropTypes.string.isRequired, - description: PropTypes.string.isRequired -} - -export default HotkeysModalContent diff --git a/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal.js b/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal.js index 881e7b060a..e9904e01a3 100644 --- a/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal.js +++ b/services/web/frontend/js/features/hotkeys-modal/components/hotkeys-modal.js @@ -1,26 +1,185 @@ import React from 'react' -import { Modal } from 'react-bootstrap' +import { Button, Modal, Row, Col } from 'react-bootstrap' import PropTypes from 'prop-types' -import HotkeysModalContent from './hotkeys-modal-content' +import { Trans, useTranslation } from 'react-i18next' +import AccessibleModal from '../../../shared/components/accessible-modal' -function HotkeysModal({ handleHide, show, trackChangesVisible = false }) { - const isMac = /Mac/i.test(navigator.platform) +export default function HotkeysModal({ + animation = true, + handleHide, + show, + isMac = false, + trackChangesVisible = false +}) { + const { t } = useTranslation() + + const ctrl = isMac ? 'Cmd' : 'Ctrl' return ( - - - + + + {t('hotkeys')} + + + +

{t('common')}

+ + + + + + + + + + + + + + +

{t('navigation')}

+ + + + + + + + + + + + + +

{t('editing')}

+ + + + + + + + + + + + + + + + + + + + +

{t('autocomplete')}

+ + + + + + + + + + + + + +

+ }} + /> +

+ + + + + + + + {trackChangesVisible && ( + <> +

{t('review')}

+ + + + + + + + + + + + + + )} + + + + + + ) } HotkeysModal.propTypes = { - handleHide: PropTypes.func.isRequired, + animation: PropTypes.bool, + isMac: PropTypes.bool, show: PropTypes.bool.isRequired, + handleHide: PropTypes.func.isRequired, trackChangesVisible: PropTypes.bool } -export default HotkeysModal +function Hotkey({ combination, description }) { + return ( +
+ {combination} + {description} +
+ ) +} +Hotkey.propTypes = { + combination: PropTypes.string.isRequired, + description: PropTypes.string.isRequired +} diff --git a/services/web/frontend/js/features/hotkeys-modal/controllers/hotkeys-modal-controller.js b/services/web/frontend/js/features/hotkeys-modal/controllers/hotkeys-modal-controller.js index d28c4dc186..8dbea70cee 100644 --- a/services/web/frontend/js/features/hotkeys-modal/controllers/hotkeys-modal-controller.js +++ b/services/web/frontend/js/features/hotkeys-modal/controllers/hotkeys-modal-controller.js @@ -3,10 +3,11 @@ import { react2angular } from 'react2angular' import HotkeysModal from '../components/hotkeys-modal' -App.component('hotkeysModal', react2angular(HotkeysModal)) +App.component('hotkeysModal', react2angular(HotkeysModal, undefined)) export default App.controller('HotkeysModalController', function($scope) { $scope.show = false + $scope.isMac = /Mac/i.test(navigator.platform) $scope.handleHide = () => { $scope.$applyAsync(() => { @@ -15,10 +16,10 @@ export default App.controller('HotkeysModalController', function($scope) { } $scope.openHotkeysModal = () => { - $scope.trackChangesVisible = - $scope.project && $scope.project.features.trackChangesVisible - $scope.$applyAsync(() => { + $scope.trackChangesVisible = + $scope.project && $scope.project.features.trackChangesVisible + $scope.show = true }) } diff --git a/services/web/frontend/stories/hotkeys-modal.stories.js b/services/web/frontend/stories/hotkeys-modal.stories.js index e7fdf1e505..137019b4ef 100644 --- a/services/web/frontend/stories/hotkeys-modal.stories.js +++ b/services/web/frontend/stories/hotkeys-modal.stories.js @@ -1,21 +1,26 @@ import React from 'react' -import HotkeysModalContent from '../js/features/hotkeys-modal/components/hotkeys-modal-content' +import HotkeysModal from '../js/features/hotkeys-modal/components/hotkeys-modal' -// NOTE: HotkeysModalContent is wrapped in modal classes, without modal behaviours -export const Basic = args => ( -
-
- -
-
-) +export const ReviewEnabled = args => { + return +} + +export const ReviewDisabled = args => { + return +} + +export const MacModifier = args => { + return +} export default { title: 'Hotkeys Modal', - component: HotkeysModalContent, + component: HotkeysModal, args: { - isMac: true, + animation: false, + show: true, + isMac: false, trackChangesVisible: true }, argTypes: { diff --git a/services/web/test/frontend/features/hotkeys-modal/components/hotkeys-modal.test.js b/services/web/test/frontend/features/hotkeys-modal/components/hotkeys-modal.test.js index 1065d0ff7a..0c56c528c3 100644 --- a/services/web/test/frontend/features/hotkeys-modal/components/hotkeys-modal.test.js +++ b/services/web/test/frontend/features/hotkeys-modal/components/hotkeys-modal.test.js @@ -1,61 +1,62 @@ import React from 'react' import { render, screen } from '@testing-library/react' -import HotkeysModalContent from '../../../../../frontend/js/features/hotkeys-modal/components/hotkeys-modal-content' +import HotkeysModal from '../../../../../frontend/js/features/hotkeys-modal/components/hotkeys-modal' import { expect } from 'chai' +import sinon from 'sinon' -const handleHide = () => { - // closed +const modalProps = { + show: true, + handleHide: sinon.stub(), + trackChangesVisible: false } -describe('', function() { - it('renders the translated modal title', function() { - const { container } = render( - - ) +describe('', function() { + it('renders the translated modal title', async function() { + const { baseElement } = render() - expect(container.querySelector('.modal-title').textContent).to.equal( + expect(baseElement.querySelector('.modal-title').textContent).to.equal( 'Hotkeys' ) }) it('renders translated heading with embedded code', function() { - const { container } = render( - - ) + const { baseElement } = render() - const results = container.querySelectorAll('h3 code') + const results = baseElement.querySelectorAll('h3 code') expect(results).to.have.length(1) }) it('renders the hotkey descriptions', function() { - const { container } = render( - - ) + const { baseElement } = render() - const hotkeys = container.querySelectorAll('[data-test-selector="hotkey"]') + const hotkeys = baseElement.querySelectorAll( + '[data-test-selector="hotkey"]' + ) expect(hotkeys).to.have.length(19) }) - it('renders extra hotkey descriptions when Track Changes is enabled', function() { - const { container } = render( - + it('adds extra hotkey descriptions when Track Changes is enabled', function() { + const { baseElement } = render( + ) - const hotkeys = container.querySelectorAll('[data-test-selector="hotkey"]') + const hotkeys = baseElement.querySelectorAll( + '[data-test-selector="hotkey"]' + ) expect(hotkeys).to.have.length(22) }) it('uses Ctrl for non-macOS', function() { - render() + render() - screen.getAllByText(/Ctrl/) + expect(screen.getAllByText(/Ctrl/)).to.have.length(16) expect(screen.queryByText(/Cmd/)).to.not.exist }) it('uses Cmd for macOS', function() { - render() + render() - screen.getAllByText(/Cmd/) + expect(screen.getAllByText(/Cmd/)).to.have.length(16) expect(screen.queryByText(/Ctrl/)).to.not.exist }) })