feat(sidebar): Add support for toggling the sidebar externally (#1293)

jstoffan · mergify[bot] · commit 04468fc22200 · 2019-06-19T20:25:58.000Z
diff --git a/src/elements/common/nav-button/NavButton.js b/src/elements/common/nav-button/NavButton.js
@@ -55,7 +55,7 @@ const NavButton = React.forwardRef<Props, React.Ref<any>>((props: Props, ref: Re
 
                             if (!event.defaultPrevented && isLeftClick(event)) {
                                 const method = replace ? history.replace : history.push;
-                                method(path);
+                                method(to);
                             }
                         }}
                         ref={ref}
diff --git a/src/elements/content-sidebar/Sidebar.js b/src/elements/content-sidebar/Sidebar.js
@@ -7,6 +7,7 @@
 import * as React from 'react';
 import classNames from 'classnames';
 import flow from 'lodash/flow';
+import getProp from 'lodash/get';
 import uniqueid from 'lodash/uniqueId';
 import { withRouter } from 'react-router-dom';
 import type { Location, RouterHistory } from 'react-router-dom';
@@ -48,76 +49,51 @@ type Props = {
 
 type State = {
     isDirty: boolean,
-    isOpen: boolean, // Local isOpen state consists of stored forced state (if any) and responsive adjustments
 };
 
 export const SIDEBAR_FORCE_KEY: 'bcs.force' = 'bcs.force';
 export const SIDEBAR_FORCE_VALUE_CLOSED: 'closed' = 'closed';
 export const SIDEBAR_FORCE_VALUE_OPEN: 'open' = 'open';
 
 class Sidebar extends React.Component<Props, State> {
+    static defaultProps = {
+        isLarge: true,
+        isLoading: false,
+    };
+
     id: string = uniqueid('bcs_');
 
     props: Props;
 
-    state: State;
+    state: State = {
+        isDirty: false,
+    };
 
     store: LocalStore = new LocalStore();
 
-    static defaultProps = {
-        isLarge: true,
-        isLoading: false,
-    };
-
     constructor(props: Props) {
         super(props);
 
-        const { isLarge } = this.props;
-
-        this.state = {
-            isDirty: false,
-            isOpen: this.isForcedSet() ? this.isForcedOpen() : !!isLarge,
-        };
+        this.setForcedByLocation();
     }
 
     componentDidUpdate(prevProps: Props): void {
-        const { fileId, history, isLarge, location }: Props = this.props;
-        const { fileId: prevFileId, isLarge: prevIsLarge, location: prevLocation }: Props = prevProps;
-        const { isDirty, isOpen }: State = this.state;
-        const { state: locationState = {} } = location;
-        const isForcedSet = this.isForcedSet();
+        const { fileId, history, location }: Props = this.props;
+        const { fileId: prevFileId, location: prevLocation }: Props = prevProps;
+        const { isDirty }: State = this.state;
 
-        // User navigated to a different file without ever navigating to a tab
+        // User navigated to a different file without ever navigating the sidebar
         if (!isDirty && fileId !== prevFileId && location.pathname !== '/') {
             history.replace({ pathname: '/', state: { silent: true } });
         }
 
-        // User navigated to a different route or tab for the first time this session
-        if (!isDirty && location !== prevLocation && !locationState.silent) {
+        // User navigated or toggled the sidebar intentionally, internally or externally
+        if (location !== prevLocation && !this.getLocationState('silent')) {
+            this.setForcedByLocation();
             this.setState({ isDirty: true });
         }
-
-        // User resized their viewport without ever toggling the sidebar open/closed
-        if (!isForcedSet && isLarge !== prevIsLarge && isLarge !== isOpen) {
-            this.setState({ isOpen: isLarge });
-        }
     }
 
-    /**
-     * Handle sidebar navigation events
-     *
-     * @param {SyntheticEvent} event - The event
-     * @param {NavigateOptions} options - The navigation options
-     * @return {void}
-     */
-    handleNavigation = (event: SyntheticEvent<>, { isToggle }: NavigateOptions): void => {
-        const { isOpen }: State = this.state;
-
-        // Persist user preference for all future sessions in this browser
-        this.isForced(isToggle ? !isOpen : true);
-        this.setState({ isOpen: this.isForcedOpen() });
-    };
-
     /**
      * Handle version history click
      *
@@ -136,6 +112,22 @@ class Sidebar extends React.Component<Props, State> {
         history.push(`${history.location.pathname}/versions${fileVersionSlug}`);
     };
 
+    /**
+     * Getter for location state properties.
+     *
+     * NOTE: Each location on the history stack has its own optional state object that is wholly separate from
+     * this component's internal state. Values on the location state object can persist even between refreshes
+     * when using certain history contexts, such as BrowserHistory.
+     *
+     * @param key - Optionally get a specific key value from state
+     * @returns {any} - The location state or state key value
+     */
+    getLocationState(key?: string): any {
+        const { location } = this.props;
+        const { state: locationState = {} } = location;
+        return getProp(locationState, key);
+    }
+
     /**
      * Getter/setter for sidebar forced state
      *
@@ -155,7 +147,7 @@ class Sidebar extends React.Component<Props, State> {
      * @returns {boolean} - True if the sidebar has been forced open
      */
     isForcedOpen(): boolean {
-        return this.isForced() !== SIDEBAR_FORCE_VALUE_CLOSED;
+        return this.isForced() === SIDEBAR_FORCE_VALUE_OPEN;
     }
 
     /**
@@ -166,6 +158,17 @@ class Sidebar extends React.Component<Props, State> {
         return this.isForced() !== null;
     }
 
+    /**
+     * Helper to set the local store open state based on the location open state, if defined
+     */
+    setForcedByLocation(): void {
+        const isLocationOpen: ?boolean = this.getLocationState('open');
+
+        if (isLocationOpen !== undefined && isLocationOpen !== null) {
+            this.isForced(isLocationOpen);
+        }
+    }
+
     render() {
         const {
             activitySidebarProps,
@@ -179,13 +182,14 @@ class Sidebar extends React.Component<Props, State> {
             getPreview,
             getViewer,
             hasAdditionalTabs,
+            isLarge,
             isLoading,
             metadataEditors,
             metadataSidebarProps,
             onVersionChange,
         }: Props = this.props;
 
-        const { isOpen } = this.state;
+        const isOpen = this.isForcedSet() ? this.isForcedOpen() : !!isLarge;
         const hasActivity = SidebarUtils.canHaveActivitySidebar(this.props);
         const hasDetails = SidebarUtils.canHaveDetailsSidebar(this.props);
         const hasMetadata = SidebarUtils.shouldRenderMetadataSidebar(this.props, metadataEditors);
@@ -213,7 +217,6 @@ class Sidebar extends React.Component<Props, State> {
                             hasMetadata={hasMetadata}
                             hasSkills={hasSkills}
                             isOpen={isOpen}
-                            onNavigate={this.handleNavigation}
                         />
                         <SidebarPanels
                             activitySidebarProps={activitySidebarProps}
diff --git a/src/elements/content-sidebar/SidebarNav.js b/src/elements/content-sidebar/SidebarNav.js
@@ -10,10 +10,10 @@ import IconMagicWand from '../../icons/general/IconMagicWand';
 import IconMetadataThick from '../../icons/general/IconMetadataThick';
 import IconDocInfo from '../../icons/general/IconDocInfo';
 import IconChatRound from '../../icons/general/IconChatRound';
-import SidebarToggleButton from '../../components/sidebar-toggle-button';
 import messages from '../common/messages';
 import { SIDEBAR_NAV_TARGETS } from '../common/interactionTargets';
 import SidebarNavButton from './SidebarNavButton';
+import SidebarToggle from './SidebarToggle';
 import AdditionalTabs from './additional-tabs';
 import {
     SIDEBAR_VIEW_SKILLS,
@@ -100,16 +100,7 @@ const SidebarNav = ({
             {hasAdditionalTabs && <AdditionalTabs key={fileId} tabs={additionalTabs} />}
         </div>
         <div className="bcs-SidebarNav-footer">
-            <SidebarToggleButton
-                data-resin-target={SIDEBAR_NAV_TARGETS.TOGGLE}
-                data-testid="sidebartoggle"
-                isOpen={isOpen}
-                onClick={event => {
-                    if (onNavigate) {
-                        onNavigate(event, { isToggle: true });
-                    }
-                }}
-            />
+            <SidebarToggle isOpen={isOpen} />
         </div>
     </div>
 );
diff --git a/src/elements/content-sidebar/SidebarNavButton.js b/src/elements/content-sidebar/SidebarNavButton.js
@@ -15,7 +15,6 @@ type Props = {
     'data-testid'?: string,
     children: React.Node,
     isOpen?: boolean,
-    onNavigate?: (SyntheticEvent<>, NavigateOptions) => void,
     sidebarView: string,
     tooltip: React.Node,
 };
@@ -25,7 +24,6 @@ const SidebarNavButton = ({
     'data-resin-target': dataResinTarget,
     'data-testid': dataTestId,
     isOpen,
-    onNavigate,
     sidebarView,
     tooltip,
 }: Props) => {
@@ -37,6 +35,7 @@ const SidebarNavButton = ({
                 const isMatch = !!match;
                 const isActive = () => isMatch && !!isOpen;
                 const isToggle = isMatch && match.isExact;
+                const sidebarState = { open: isToggle ? !isOpen : true };
 
                 return (
                     <Tooltip position="middle-left" text={tooltip}>
@@ -47,14 +46,12 @@ const SidebarNavButton = ({
                             data-resin-target={dataResinTarget}
                             data-testid={dataTestId}
                             isActive={isActive}
-                            onClick={event => {
-                                if (onNavigate) {
-                                    onNavigate(event, { isToggle });
-                                }
-                            }}
                             replace={isToggle}
                             role="tab"
-                            to={sidebarPath}
+                            to={{
+                                pathname: sidebarPath,
+                                state: sidebarState,
+                            }}
                             type="button"
                         >
                             {children}
diff --git a/src/elements/content-sidebar/SidebarToggle.js b/src/elements/content-sidebar/SidebarToggle.js
@@ -0,0 +1,32 @@
+/**
+ * @flow strict
+ * @file Sidebar Toggle component
+ * @author Box
+ */
+
+import * as React from 'react';
+import { withRouter, type RouterHistory } from 'react-router-dom';
+import SidebarToggleButton from '../../components/sidebar-toggle-button/SidebarToggleButton';
+import { SIDEBAR_NAV_TARGETS } from '../common/interactionTargets';
+
+type Props = {
+    history: RouterHistory,
+    isOpen?: boolean,
+};
+
+const SidebarToggle = ({ history, isOpen }: Props) => {
+    return (
+        <SidebarToggleButton
+            data-resin-target={SIDEBAR_NAV_TARGETS.TOGGLE}
+            data-testid="sidebartoggle"
+            isOpen={isOpen}
+            onClick={event => {
+                event.preventDefault();
+                history.replace({ state: { open: !isOpen } });
+            }}
+        />
+    );
+};
+
+export { SidebarToggle as SidebarToggleComponent };
+export default withRouter(SidebarToggle);
diff --git a/src/elements/content-sidebar/__tests__/Sidebar-test.js b/src/elements/content-sidebar/__tests__/Sidebar-test.js
@@ -26,24 +26,18 @@ describe('elements/content-sidebar/Sidebar', () => {
             }));
         });
 
-        test('should set isOpen if isLarge prop has changed', () => {
-            const wrapper = getWrapper({ isLarge: true });
-
-            expect(wrapper.state('isOpen')).toEqual(true);
-
-            wrapper.setProps({ isLarge: false });
-
-            expect(wrapper.state('isOpen')).toEqual(false);
-        });
-
-        test('should set isDirty if a user-initiated location change occurred', () => {
+        test('should update if a user-initiated location change occurred', () => {
             const wrapper = getWrapper({ location: { pathname: '/activity' } });
+            const instance = wrapper.instance();
+            instance.setForcedByLocation = jest.fn();
 
             expect(wrapper.state('isDirty')).toBe(false);
+            expect(instance.setForcedByLocation).not.toHaveBeenCalled();
 
             wrapper.setProps({ location: { pathname: '/details' } });
 
             expect(wrapper.state('isDirty')).toBe(true);
+            expect(instance.setForcedByLocation).toHaveBeenCalled();
         });
 
         test('should not set isDirty if an app-initiated location change occurred', () => {
@@ -55,6 +49,24 @@ describe('elements/content-sidebar/Sidebar', () => {
 
             expect(wrapper.state('isDirty')).toBe(false);
         });
+
+        test('should set the forced open state if the location state is present', () => {
+            const wrapper = getWrapper({ location: { pathname: '/' } });
+            const instance = wrapper.instance();
+            instance.isForced = jest.fn();
+
+            wrapper.setProps({ location: { pathname: '/details' } });
+            expect(instance.isForced).toHaveBeenCalledWith(); // Getter for render
+
+            wrapper.setProps({ location: { pathname: '/details/inner', state: { open: true, silent: true } } });
+            expect(instance.isForced).toHaveBeenCalledWith(); // Getter for render
+
+            wrapper.setProps({ location: { pathname: '/', state: { open: true } } });
+            expect(instance.isForced).toHaveBeenCalledWith(true);
+
+            wrapper.setProps({ location: { pathname: '/', state: { open: false } } });
+            expect(instance.isForced).toHaveBeenCalledWith(false);
+        });
     });
 
     describe('isForced', () => {
diff --git a/src/elements/content-sidebar/__tests__/SidebarNav-test.js b/src/elements/content-sidebar/__tests__/SidebarNav-test.js
@@ -1,12 +1,15 @@
 import React from 'react';
 import { MemoryRouter } from 'react-router-dom';
 import { mount } from 'enzyme';
+import AdditionalTabPlaceholder from '../additional-tabs/AdditionalTabPlaceholder';
+import AdditionalTabs from '../additional-tabs';
+import AdditionalTabsLoading from '../additional-tabs/AdditionalTabsLoading';
+import IconChatRound from '../../../icons/general/IconChatRound';
+import IconDocInfo from '../../../icons/general/IconDocInfo';
 import IconMagicWand from '../../../icons/general/IconMagicWand';
 import IconMetadataThick from '../../../icons/general/IconMetadataThick';
-import IconDocInfo from '../../../icons/general/IconDocInfo';
-import IconChatRound from '../../../icons/general/IconChatRound';
-import SidebarNavButton from '../SidebarNavButton';
 import SidebarNav from '../SidebarNav';
+import SidebarNavButton from '../SidebarNavButton';
 
 describe('elements/content-sidebar/SidebarNav', () => {
     const getWrapper = (props, active = '') =>
@@ -78,10 +81,12 @@ describe('elements/content-sidebar/SidebarNav', () => {
 
     test('should render the additional tabs loading state', () => {
         const props = {
-            hasAdditionalTabs: true,
             additionalTabs: [],
+            hasAdditionalTabs: true,
         };
         const wrapper = getWrapper(props);
-        expect(wrapper).toMatchSnapshot();
+        expect(wrapper.find(AdditionalTabs)).toHaveLength(1);
+        expect(wrapper.find(AdditionalTabsLoading)).toHaveLength(1);
+        expect(wrapper.find(AdditionalTabPlaceholder)).toHaveLength(5);
     });
 });
diff --git a/src/elements/content-sidebar/__tests__/SidebarToggle-test.js b/src/elements/content-sidebar/__tests__/SidebarToggle-test.js
diff --git a/src/elements/content-sidebar/__tests__/__snapshots__/SidebarNav-test.js.snap b/src/elements/content-sidebar/__tests__/__snapshots__/SidebarNav-test.js.snap
diff --git a/src/elements/content-sidebar/__tests__/__snapshots__/SidebarToggle-test.js.snap b/src/elements/content-sidebar/__tests__/__snapshots__/SidebarToggle-test.js.snap

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ const NavButton = React.forwardRef<Props, React.Ref<any>>((props: Props, ref: Re`
`55`	`55`
`56`	`56`	`if (!event.defaultPrevented && isLeftClick(event)) {`
`57`	`57`	`const method = replace ? history.replace : history.push;`
`58`		`- method(path);`
	`58`	`+ method(to);`
`59`	`59`	`}`
`60`	`60`	`}}`
`61`	`61`	`ref={ref}`