From 61b6de2e4e9bcac5eb3731e7b8b5619145248259 Mon Sep 17 00:00:00 2001 From: Thibaud Colas Date: Fri, 9 Jun 2017 00:08:04 +0300 Subject: [PATCH] Tidy up new React + API explorer for mobile (fixes #3607) (#3635) * Remove useless CSS declaration * Remove commented out styles * Merge duplicate declarations * Remove even more commented out code * Move footer mq to footer declaration * Remove more useless code * Stop vendor prefixing for IE below 11 * Remove useless vendor prefixing * Merge identical declarations * Fix 1px overflow in content wrapper * Fix explorer scrolling when open on mobile * Remove unused import * Add Redux performance measurements to explorer menu * Rewrite explorer reducer to avoid unnecessary operations * Stop changing reducer state on every action regardless of type * Remove redundant children.isFetching property in nodes reducer * Remove redundant children.isLoaded property in nodes reducer * Remove redundant children.isError property in nodes reducer * Refactor nodes explorer reducer with sub-reducer * Fix linting issue * Remove unused class name * Change default icon className from empty string to null * Remove old TODO comment * Hoist icons in ExplorerItem component for better performance * Add comment * Add tooling for performance measurement of React components * Clean up explorer panel component definition * Make performance measurements opt-in * Improve alignment of page explorer menu on mobile * Close explorer on touchend rather than touchstart * Comment out performance measurement code * Remove fade transition completely --- client/src/components/Explorer/Explorer.scss | 22 ++++- .../src/components/Explorer/ExplorerItem.js | 25 ++++- .../src/components/Explorer/ExplorerItem.scss | 6 +- .../src/components/Explorer/ExplorerPanel.js | 24 +++-- .../__snapshots__/Explorer.test.js.snap | 6 -- .../__snapshots__/ExplorerHeader.test.js.snap | 6 +- .../__snapshots__/ExplorerItem.test.js.snap | 28 +++--- .../__snapshots__/ExplorerPanel.test.js.snap | 5 + .../__snapshots__/PageCount.test.js.snap | 6 +- client/src/components/Explorer/actions.js | 2 +- .../src/components/Explorer/actions.test.js | 8 +- client/src/components/Explorer/index.js | 6 ++ .../reducers/__snapshots__/nodes.test.js.snap | 31 +++--- .../components/Explorer/reducers/explorer.js | 29 +++--- .../src/components/Explorer/reducers/nodes.js | 99 +++++++++++-------- client/src/components/Icon/Icon.js | 4 +- .../src/components/Transition/Transition.js | 3 +- .../src/components/Transition/Transition.scss | 26 ----- client/src/utils/performance.js | 86 ++++++++++++++++ gulpfile.js/tasks/styles.js | 2 +- package.json | 1 + .../scss/components/_main-nav.scss | 9 +- .../static_src/wagtailadmin/scss/core.scss | 54 +--------- .../templates/wagtailadmin/admin_base.html | 1 - 24 files changed, 278 insertions(+), 211 deletions(-) create mode 100644 client/src/utils/performance.js diff --git a/client/src/components/Explorer/Explorer.scss b/client/src/components/Explorer/Explorer.scss index 1c5e2628b..fa156a9e0 100644 --- a/client/src/components/Explorer/Explorer.scss +++ b/client/src/components/Explorer/Explorer.scss @@ -3,6 +3,7 @@ $c-explorer-bg-dark: $color-grey-1; $c-explorer-bg-active: rgba(0,0,0,0.425); $c-explorer-secondary: #a5a5a5; $c-explorer-easing: cubic-bezier(0.075, 0.820, 0.165, 1.000); +$menu-footer-height: 50px; @import 'ExplorerItem'; @@ -92,7 +93,6 @@ $c-explorer-easing: cubic-bezier(0.075, 0.820, 0.165, 1.000); .c-explorer__header { display: block; - height: 50px; background-color: $c-explorer-bg-dark; border-bottom: 1px solid $c-explorer-bg-dark; color: $color-white; @@ -114,7 +114,7 @@ $c-explorer-easing: cubic-bezier(0.075, 0.820, 0.165, 1.000); } .c-explorer__header__inner { - padding: 1rem; + padding: 1em .75em; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; @@ -124,17 +124,24 @@ $c-explorer-easing: cubic-bezier(0.075, 0.820, 0.165, 1.000); margin-right: .25rem; font-size: 1rem; } + + @include medium { + padding: 1em 1.5em; + } } .c-explorer__placeholder { - padding: 1rem; + padding: 1em; color: $color-white; + + @include medium { + padding: 1em 1.75em; + } } .c-explorer__see-more { display: block; - padding: 1rem; - height: 50px; + padding: 1em; background: rgba(0,0,0,0.3); color: $color-white; @@ -152,4 +159,9 @@ $c-explorer-easing: cubic-bezier(0.075, 0.820, 0.165, 1.000); @include hover { background: $c-explorer-bg-active; } + + @include medium { + padding: 1em 1.75em; + height: $menu-footer-height; + } } diff --git a/client/src/components/Explorer/ExplorerItem.js b/client/src/components/Explorer/ExplorerItem.js index 3df7684b9..c746fae46 100644 --- a/client/src/components/Explorer/ExplorerItem.js +++ b/client/src/components/Explorer/ExplorerItem.js @@ -6,6 +6,23 @@ import Icon from '../../components/Icon/Icon'; import Button from '../../components/Button/Button'; import PublicationStatus from '../../components/PublicationStatus/PublicationStatus'; +// Hoist icons in the explorer item, as it is re-rendered many times. +const childrenIcon = ( + +); + +const editIcon = ( + +); + +const nextIcon = ( + +); + +/** + * One menu item in the page explorer, with different available actions + * and information depending on the metadata of the page. + */ const ExplorerItem = ({ item, onClick }) => { const { id, title, meta } = item; const hasChildren = meta.children.count > 0; @@ -14,9 +31,7 @@ const ExplorerItem = ({ item, onClick }) => { return (
{hasChildren ? ( ) : null}
diff --git a/client/src/components/Explorer/ExplorerItem.scss b/client/src/components/Explorer/ExplorerItem.scss index 385a63c3f..6d5414e38 100644 --- a/client/src/components/Explorer/ExplorerItem.scss +++ b/client/src/components/Explorer/ExplorerItem.scss @@ -8,7 +8,7 @@ display: inline-flex; align-items: center; flex-grow: 1; - padding: 1.45em 1.75em; + padding: 1.45em 1em; cursor: pointer; &:focus { @@ -25,6 +25,10 @@ @include hover { background: $c-explorer-bg-active; } + + @include medium { + padding: 1.45em 1.75em; + } } .c-explorer__item__link .icon { diff --git a/client/src/components/Explorer/ExplorerPanel.js b/client/src/components/Explorer/ExplorerPanel.js index ae3e9b1b9..89cdb90c7 100644 --- a/client/src/components/Explorer/ExplorerPanel.js +++ b/client/src/components/Explorer/ExplorerPanel.js @@ -6,12 +6,16 @@ import { STRINGS, MAX_EXPLORER_PAGES } from '../../config/wagtailConfig'; import Button from '../Button/Button'; import LoadingSpinner from '../LoadingSpinner/LoadingSpinner'; -import Transition, { PUSH, POP, FADE } from '../Transition/Transition'; +import Transition, { PUSH, POP } from '../Transition/Transition'; import ExplorerHeader from './ExplorerHeader'; import ExplorerItem from './ExplorerItem'; import PageCount from './PageCount'; -export default class ExplorerPanel extends React.Component { +/** + * The main panel of the page explorer menu, with heading, + * menu items, and special states. + */ +class ExplorerPanel extends React.Component { constructor(props) { super(props); @@ -38,14 +42,14 @@ export default class ExplorerPanel extends React.Component { document.querySelector('[data-explorer-menu-item]').classList.add('submenu-active'); document.body.classList.add('explorer-open'); document.addEventListener('mousedown', this.clickOutside); - document.addEventListener('touchstart', this.clickOutside); + document.addEventListener('touchend', this.clickOutside); } componentWillUnmount() { document.querySelector('[data-explorer-menu-item]').classList.remove('submenu-active'); document.body.classList.remove('explorer-open'); document.removeEventListener('mousedown', this.clickOutside); - document.removeEventListener('touchstart', this.clickOutside); + document.removeEventListener('touchend', this.clickOutside); } clickOutside(e) { @@ -136,7 +140,10 @@ export default class ExplorerPanel extends React.Component { tag="nav" className="explorer" paused={paused || !page || page.isFetching} - focusTrapOptions={{ onDeactivate: onClose }} + focusTrapOptions={{ + initialFocus: '.c-explorer__close', + onDeactivate: onClose, + }} > @@ -114,7 +114,7 @@ exports[`ExplorerItem should show a publication status if not live 1`] = ` target={null} > @@ -148,9 +148,9 @@ exports[`ExplorerItem should show a publication status if not live 1`] = ` target={null} >