From 29b39d1b616c8f7733b20e3c8a1aa1bb3b268787 Mon Sep 17 00:00:00 2001 From: Mikkel Laursen Date: Sun, 12 Mar 2017 20:25:38 -0600 Subject: [PATCH] Fixed Drawer/NavigationDrawer visibilities My initial fix in #286 was not actually the real solution since it caused #288 The visibility will now be changed if there is a resize event after the initial mount OR the defaultVisible is false and prop.visible is false. This should now close #288 --- src/js/Drawers/Drawer.js | 50 ++++-- src/js/Drawers/__tests__/Drawer.js | 154 +++++++++++++--- src/js/NavigationDrawers/NavigationDrawer.js | 22 +-- .../__tests__/NavigationDrawer.js | 167 ++++++++++++++---- 4 files changed, 298 insertions(+), 95 deletions(-) diff --git a/src/js/Drawers/Drawer.js b/src/js/Drawers/Drawer.js index 67b33d0e67..95b33d4dbe 100644 --- a/src/js/Drawers/Drawer.js +++ b/src/js/Drawers/Drawer.js @@ -223,10 +223,10 @@ export default class Drawer extends PureComponent { /** * An optional function to call when the visibility of the drawer is changed. The function will - * be called with the new visibility state and an event that triggered the visibility change. + * be called with the new visibility statee. * * ```js - * onVisibilityToggle(!currentlyVisible, event); + * onVisibilityToggle(!currentlyVisible); * ``` */ onVisibilityToggle: PropTypes.func, @@ -350,6 +350,7 @@ export default class Drawer extends PureComponent { } const type = getField(props, this.state, 'type'); + this._initialFix = true; if (typeof props.visible === 'undefined') { this.state.visible = typeof defaultVisible !== 'undefined' @@ -372,9 +373,7 @@ export default class Drawer extends PureComponent { componentDidMount() { window.addEventListener('resize', this._updateMedia); - if (typeof this.props.visible === 'undefined' && !this.props.defaultVisible) { - this._updateType(this.props); - } + this._updateType(this.props); } componentWillReceiveProps(nextProps) { @@ -431,25 +430,38 @@ export default class Drawer extends PureComponent { const state = Drawer.getCurrentMedia(props); const diffType = getField(props, this.state, 'type') !== state.type; + const diffMedia = state.mobile !== this.state.mobile + || state.tablet !== this.state.tablet + || state.desktop !== this.state.desktop; - if (onMediaTypeChange && (diffType || - ['mobile', 'tablet', 'desktop'].filter(key => state[key] !== this.state[key]).length) - ) { + if (onMediaTypeChange && (diffType || diffMedia)) { onMediaTypeChange(state.type, { mobile: state.mobile, tablet: state.tablet, desktop: state.desktop }); } - if (typeof props.type !== 'undefined') { - delete state.type; - } + if (diffType) { + let visible = isPermanent(state.type); + if (this._initialFix) { + if (props.defaultVisible) { + visible = props.defaultVisible; + } else if (props.visible) { + visible = props.visible; + } - const type = getField(props, state, 'type'); - const visible = isPermanent(type); - if (onVisibilityToggle && getField(props, this.state, 'visible') !== visible) { - onVisibilityToggle(visible); + this._initialFix = false; + } + const prevVisible = getField(props, this.state, 'visible'); + + if (onVisibilityToggle && (visible !== prevVisible)) { + onVisibilityToggle(visible); + } + + if (typeof props.visible === 'undefined') { + state.visible = visible; + } } - if (typeof props.visible === 'undefined' && diffType) { - state.visible = visible; + if (typeof props.type !== 'undefined') { + delete state.type; } this.setState(state); @@ -509,9 +521,9 @@ export default class Drawer extends PureComponent { } } - _closeDrawer(e) { + _closeDrawer() { if (this.props.onVisibilityToggle) { - this.props.onVisibilityToggle(false, e); + this.props.onVisibilityToggle(false); } if (typeof this.props.visible === 'undefined') { diff --git a/src/js/Drawers/__tests__/Drawer.js b/src/js/Drawers/__tests__/Drawer.js index 28147e2cd9..5eadcd90d4 100644 --- a/src/js/Drawers/__tests__/Drawer.js +++ b/src/js/Drawers/__tests__/Drawer.js @@ -1,7 +1,10 @@ /* eslint-env jest */ +/* eslint-disable max-len */ jest.unmock('../Drawer'); -jest.unmock('../../Dialogs/Dialog'); +jest.unmock('../DrawerTypes'); jest.unmock('../isType'); +jest.unmock('../../constants/media'); +jest.unmock('../../Dialogs/Dialog'); jest.unmock('../../Papers/Paper'); import React from 'react'; @@ -21,33 +24,132 @@ describe('Drawer', () => { expect(drawer.context.renderNode).toBe(renderNode); }); - it('should call the updateType function after initial mount', () => { - const onVisibilityToggle = jest.fn(); - renderIntoDocument( - - ); + describe('updateType', () => { + const MATCH_MEDIA = window.matchMedia; + const matchesMobile = jest.fn(query => ({ + matches: query.indexOf(Drawer.defaultProps.mobileMinWidth) !== -1, + })); + const matchesTablet = jest.fn(query => ({ + matches: query.indexOf(Drawer.defaultProps.tabletMinWidth) !== -1, + })); + const matchesDesktop = jest.fn(query => ({ + matches: query.indexOf('max') === -1 + && query.indexOf(Drawer.defaultProps.desktopMinWidth) !== -1, + })); + afterAll(() => { + window.matchMedia = MATCH_MEDIA; + }); - expect(onVisibilityToggle.mock.calls.length).toBe(1); - }); + it('should correctly set the default visibility on mobile devices', () => { + const props = { + navItems: [], + mobileType: Drawer.DrawerTypes.TEMPORARY, + tabletType: Drawer.DrawerTypes.PERSISTENT, + desktopType: Drawer.DrawerTypes.FULL_HEIGHT, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesMobile; + const drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(false); + expect(drawer.state.type).toBe(Drawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(0); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + }); - it('should not call the updateType function after mounting if defaultVisible is true', () => { - const onVisibilityToggle = jest.fn(); - const onMediaTypeChange = jest.fn(); - const drawer = renderIntoDocument( - - ); - - expect(onVisibilityToggle.mock.calls.length).toBe(0); - expect(onMediaTypeChange.mock.calls.length).toBe(0); - expect(drawer.state.visible).toBe(true); + it('should correctly set the default visibility on tablets', () => { + const props = { + navItems: [], + mobileType: Drawer.DrawerTypes.TEMPORARY, + tabletType: Drawer.DrawerTypes.PERSISTENT, + desktopType: Drawer.DrawerTypes.FULL_HEIGHT, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesTablet; + const drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(false); + expect(drawer.state.type).toBe(Drawer.DrawerTypes.PERSISTENT); + expect(props.onMediaTypeChange.mock.calls.length).toBe(1); + expect(props.onMediaTypeChange).toBeCalledWith(Drawer.DrawerTypes.PERSISTENT, { mobile: false, tablet: true, desktop: false }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + }); + + it('should correctly set the default visibility on desktop', () => { + const props = { + navItems: [], + mobileType: Drawer.DrawerTypes.TEMPORARY, + tabletType: Drawer.DrawerTypes.PERSISTENT, + desktopType: Drawer.DrawerTypes.FULL_HEIGHT, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesDesktop; + const drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.type).toBe(Drawer.DrawerTypes.FULL_HEIGHT); + expect(props.onMediaTypeChange.mock.calls.length).toBe(1); + expect(props.onMediaTypeChange).toBeCalledWith(Drawer.DrawerTypes.FULL_HEIGHT, { mobile: false, tablet: false, desktop: true }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(1); + expect(props.onVisibilityToggle).toBeCalledWith(true); + }); + + it('should not update the visibility to false when the defaultVisible prop is enabled and the drawer type is temporary for any screen size', () => { + const props = { + defaultVisible: true, + navItems: [], + mobileType: Drawer.DrawerTypes.TEMPORARY, + tabletType: Drawer.DrawerTypes.TEMPORARY, + desktopType: Drawer.DrawerTypes.TEMPORARY, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesMobile; + let drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.type).toBe(Drawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(0); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + + window.matchMedia = matchesTablet; + drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.type).toBe(Drawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(1); + expect(props.onMediaTypeChange).toBeCalledWith(props.tabletType, { mobile: false, tablet: true, desktop: false }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + + window.matchMedia = matchesDesktop; + drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.type).toBe(Drawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(2); + expect(props.onMediaTypeChange).toBeCalledWith(props.desktopType, { mobile: false, tablet: false, desktop: true }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + }); + + it('should correctly update the visibility when the visible prop was defined and there was a media type change with visibility', () => { + const props = { + visible: false, + defaultMedia: 'mobile', + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesDesktop; + renderIntoDocument(); + expect(props.onMediaTypeChange).toBeCalledWith(Drawer.defaultProps.desktopType, { mobile: false, tablet: false, desktop: true }); + expect(props.onVisibilityToggle).toBeCalledWith(true); + + window.matchMedia = matchesMobile; + renderIntoDocument(); + expect(props.onMediaTypeChange).toBeCalledWith(Drawer.defaultProps.mobileType, { mobile: true, tablet: false, desktop: false }); + expect(props.onVisibilityToggle).toBeCalledWith(true); + }); }); describe('matchesMedia', () => { diff --git a/src/js/NavigationDrawers/NavigationDrawer.js b/src/js/NavigationDrawers/NavigationDrawer.js index bdd7e487ce..1860cc0e7b 100644 --- a/src/js/NavigationDrawers/NavigationDrawer.js +++ b/src/js/NavigationDrawers/NavigationDrawer.js @@ -368,10 +368,10 @@ export default class NavigationDrawer extends PureComponent { /** * An optional function to call when the visibility of the drawer changes. The callback - * will include the new visibility and the event that triggered the change. + * will include the new visibility. * * ```js - * this.props.onVisibilityToggle(false, event); + * this.props.onVisibilityToggle(false); * ``` */ onVisibilityToggle: PropTypes.func, @@ -728,9 +728,9 @@ export default class NavigationDrawer extends PureComponent { } } - _handleVisibility(visible, e) { + _handleVisibility(visible) { if (this.props.onVisibilityToggle) { - this.props.onVisibilityToggle(visible, e); + this.props.onVisibilityToggle(visible); } if (typeof this.props.visible === 'undefined') { @@ -740,24 +740,16 @@ export default class NavigationDrawer extends PureComponent { _handleTypeChange(drawerType, mediaState) { const { onMediaTypeChange } = this.props; + let state = mediaState; if (onMediaTypeChange) { onMediaTypeChange(drawerType, mediaState); } if (typeof this.props.drawerType === 'undefined') { - mediaState.drawerType = drawerType; + state = { ...mediaState, drawerType }; } - const { mobile, tablet, desktop } = this.state; - if (typeof this.props.visible === 'undefined' - && (mediaState.mobile !== mobile - || mediaState.tablet !== tablet - || mediaState.desktop !== desktop) - ) { - mediaState.visible = isPermanent(drawerType); - } - - this.setState(mediaState); + this.setState(state); } render() { diff --git a/src/js/NavigationDrawers/__tests__/NavigationDrawer.js b/src/js/NavigationDrawers/__tests__/NavigationDrawer.js index 5e9dbc7eae..d7b4fb1167 100644 --- a/src/js/NavigationDrawers/__tests__/NavigationDrawer.js +++ b/src/js/NavigationDrawers/__tests__/NavigationDrawer.js @@ -1,56 +1,153 @@ /* eslint-env jest */ +/* eslint-disable max-len */ jest.unmock('../NavigationDrawer'); jest.unmock('../../Dialogs/Dialog'); +jest.unmock('../../Drawers/Drawer'); +jest.unmock('../../Drawers/DrawerTypes'); +jest.unmock('../../Drawers/isType'); import React from 'react'; -import { findDOMNode } from 'react-dom'; import { renderIntoDocument, findRenderedComponentWithType, } from 'react-addons-test-utils'; import NavigationDrawer from '../NavigationDrawer'; -import Toolbar from '../../Toolbars/Toolbar'; import Drawer from '../../Drawers/Drawer'; import Dialog from '../../Dialogs/Dialog'; -import CSSTransitionGroup from 'react-addons-css-transition-group'; -// Not sure what to _really_ test here. describe('NavigationDrawer', () => { - it('passes styles and classnames correctly', () => { - const props = { - style: { background: 'black' }, - className: 'woop-woop', - toolbarStyle: { background: 'red' }, - toolbarClassName: 'thats-the-sound', - drawerStyle: { background: 'blue' }, - drawerClassName: 'of-the-police', - contentStyle: { background: 'orange' }, - contentClassName: 'testing', - }; - - const navigation = renderIntoDocument(); - const navigationNode = findDOMNode(navigation); - const toolbar = findRenderedComponentWithType(navigation, Toolbar); - const drawer = findRenderedComponentWithType(navigation, Drawer); - const content = findRenderedComponentWithType(navigation, CSSTransitionGroup); - - expect(navigationNode.style.background).toBe(props.style.background); - expect(navigationNode.className).toContain(props.className); - - expect(toolbar.props.style).toEqual(props.toolbarStyle); - expect(toolbar.props.className).toContain(props.toolbarClassName); - - expect(drawer.props.style).toEqual(props.drawerStyle); - expect(drawer.props.className).toContain(props.drawerClassName); - - expect(content.props.style).toEqual(props.contentStyle); - expect(content.props.className).toContain(props.contentClassName); - }); - it('should inherit the dialog\'s renderNode context', () => { const dialog = renderIntoDocument(); const drawer = findRenderedComponentWithType(dialog, NavigationDrawer); expect(drawer.context.renderNode).toBe(dialog.getChildContext().renderNode); }); + + describe('Drawer', () => { + const MATCH_MEDIA = window.matchMedia; + const matchesMobile = jest.fn(query => ({ + matches: query.indexOf(Drawer.defaultProps.mobileMinWidth) !== -1, + })); + const matchesTablet = jest.fn(query => ({ + matches: query.indexOf(Drawer.defaultProps.tabletMinWidth) !== -1, + })); + const matchesDesktop = jest.fn(query => ({ + matches: query.indexOf('max') === -1 + && query.indexOf(Drawer.defaultProps.desktopMinWidth) !== -1, + })); + afterAll(() => { + window.matchMedia = MATCH_MEDIA; + }); + + it('should correctly set the default visibility on mobile devices', () => { + const props = { + navItems: [], + mobileDrawerType: Drawer.DrawerTypes.TEMPORARY, + tabletDrawerType: Drawer.DrawerTypes.PERSISTENT, + desktopDrawerType: Drawer.DrawerTypes.FULL_HEIGHT, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesMobile; + const drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(false); + expect(drawer.state.drawerType).toBe(NavigationDrawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(0); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + }); + + it('should correctly set the default visibility on tablets', () => { + const props = { + navItems: [], + mobileDrawerType: Drawer.DrawerTypes.TEMPORARY, + tabletDrawerType: Drawer.DrawerTypes.PERSISTENT, + desktopDrawerType: Drawer.DrawerTypes.FULL_HEIGHT, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesTablet; + const drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(false); + expect(drawer.state.drawerType).toBe(NavigationDrawer.DrawerTypes.PERSISTENT); + expect(props.onMediaTypeChange.mock.calls.length).toBe(1); + expect(props.onMediaTypeChange).toBeCalledWith(NavigationDrawer.DrawerTypes.PERSISTENT, { mobile: false, tablet: true, desktop: false }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + }); + + it('should correctly set the default visibility on desktop', () => { + const props = { + navItems: [], + mobileDrawerType: Drawer.DrawerTypes.TEMPORARY, + tabletDrawerType: Drawer.DrawerTypes.PERSISTENT, + desktopDrawerType: Drawer.DrawerTypes.FULL_HEIGHT, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesDesktop; + const drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.drawerType).toBe(NavigationDrawer.DrawerTypes.FULL_HEIGHT); + expect(props.onMediaTypeChange.mock.calls.length).toBe(1); + expect(props.onMediaTypeChange).toBeCalledWith(NavigationDrawer.DrawerTypes.FULL_HEIGHT, { mobile: false, tablet: false, desktop: true }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(1); + expect(props.onVisibilityToggle).toBeCalledWith(true); + }); + + it('should not update the visibility to false when the defaultVisible prop is enabled and the drawer type is temporary for any screen size', () => { + const props = { + defaultVisible: true, + navItems: [], + mobileDrawerType: Drawer.DrawerTypes.TEMPORARY, + tabletDrawerType: Drawer.DrawerTypes.TEMPORARY, + desktopDrawerType: Drawer.DrawerTypes.TEMPORARY, + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesMobile; + let drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.drawerType).toBe(NavigationDrawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(0); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + + window.matchMedia = matchesTablet; + drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.drawerType).toBe(NavigationDrawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(1); + expect(props.onMediaTypeChange).toBeCalledWith(props.tabletDrawerType, { mobile: false, tablet: true, desktop: false }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + + window.matchMedia = matchesDesktop; + drawer = renderIntoDocument(); + expect(drawer.state.visible).toBe(true); + expect(drawer.state.drawerType).toBe(NavigationDrawer.DrawerTypes.TEMPORARY); + expect(props.onMediaTypeChange.mock.calls.length).toBe(2); + expect(props.onMediaTypeChange).toBeCalledWith(props.desktopDrawerType, { mobile: false, tablet: false, desktop: true }); + expect(props.onVisibilityToggle.mock.calls.length).toBe(0); + }); + + it('should correctly update the visibility when the visible prop was defined and there was a media type change with visibility', () => { + const props = { + visible: false, + defaultMedia: 'mobile', + onMediaTypeChange: jest.fn(), + onVisibilityToggle: jest.fn(), + }; + + window.matchMedia = matchesDesktop; + renderIntoDocument(); + expect(props.onMediaTypeChange).toBeCalledWith(NavigationDrawer.defaultProps.desktopDrawerType, { mobile: false, tablet: false, desktop: true }); + expect(props.onVisibilityToggle).toBeCalledWith(true); + + window.matchMedia = matchesMobile; + renderIntoDocument(); + expect(props.onMediaTypeChange).toBeCalledWith(NavigationDrawer.defaultProps.mobileDrawerType, { mobile: true, tablet: false, desktop: false }); + expect(props.onVisibilityToggle).toBeCalledWith(true); + }); + }); });