From 97bb2faddf9e08237290b5ca4e93d0eee1ba73b0 Mon Sep 17 00:00:00 2001 From: Kyle Durand Date: Wed, 8 Jan 2020 15:44:38 -0500 Subject: [PATCH 1/3] Fix nav hide bug --- UNRELEASED.md | 1 + src/components/Frame/Frame.tsx | 9 +- src/components/Frame/tests/Frame.test.tsx | 357 ++++++++++++---------- 3 files changed, 210 insertions(+), 157 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index 82541e3c471..d8972423950 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -8,6 +8,7 @@ ### Bug fixes +- Fixed a bug where `Navigation` calls `onNavigationDismiss` on large screens when focused and the escape key is pressed ([#2607](https://github.com/Shopify/polaris-react/pull/2607)) - Fixed issue with `Filters` component displaying an undesired margin top and bottom on the button element on Safari ([#2292](https://github.com/Shopify/polaris-react/pull/2292)) - Doesn't render `MenuActions` if no actions are passed to an `actionGroups` item inside `Page` ([#2266](https://github.com/Shopify/polaris-react/pull/2266)) - Fixed `PositionedOverlay` incorrectly calculating `Topbar.UserMenu` `Popover` width ([#1692](https://github.com/Shopify/polaris-react/pull/1692)) diff --git a/src/components/Frame/Frame.tsx b/src/components/Frame/Frame.tsx index 969437b3ca6..ec3df2c9b23 100644 --- a/src/components/Frame/Frame.tsx +++ b/src/components/Frame/Frame.tsx @@ -391,8 +391,15 @@ class Frame extends React.PureComponent { private handleNavKeydown = (event: React.KeyboardEvent) => { const {key} = event; + const { + polaris: { + mediaQuery: {isNavigationCollapsed}, + }, + showMobileNavigation, + } = this.props; - if (key === 'Escape') { + const mobileNavShowing = isNavigationCollapsed && showMobileNavigation; + if (mobileNavShowing && key === 'Escape') { this.handleNavigationDismiss(); } }; diff --git a/src/components/Frame/tests/Frame.test.tsx b/src/components/Frame/tests/Frame.test.tsx index d39c42c55c5..83c8410a913 100644 --- a/src/components/Frame/tests/Frame.test.tsx +++ b/src/components/Frame/tests/Frame.test.tsx @@ -1,7 +1,7 @@ import React from 'react'; import {CSSTransition} from '@material-ui/react-transition-group'; import {animationFrame} from '@shopify/jest-dom-mocks'; -import {documentHasStyle} from 'test-utilities'; +import {documentHasStyle, mountWithApp} from 'test-utilities'; // eslint-disable-next-line no-restricted-imports import {mountWithAppProvider, trigger} from 'test-utilities/legacy'; import { @@ -9,7 +9,7 @@ import { ContextualSaveBar as PolarisContextualSavebar, Loading as PolarisLoading, } from 'components'; -import Frame from '../Frame'; +import Frame, {FrameProps} from '../Frame'; import { ContextualSaveBar as FrameContextualSavebar, Loading as FrameLoading, @@ -41,196 +41,241 @@ describe('', () => { }); }); - it('renders a TrapFocus with a `trapping` prop set to true around the navigation on small screens and showMobileNavigation is true', () => { - const navigation =
; - const frame = mountWithAppProvider( - , - {mediaQuery: {isNavigationCollapsed: true}}, - ).find(Frame); + describe('skipToContentTarget', () => { + it('renders a skip to content link with the proper text', () => { + const skipToContentLinkText = mountWithAppProvider() + .find('a') + .at(0) + .text(); - const trapFocus = frame.find(TrapFocus); + expect(skipToContentLinkText).toStrictEqual('Skip to content'); + }); - expect(trapFocus.exists()).toBe(true); - expect(trapFocus.prop('trapping')).toBe(true); - }); + it('sets focus to the main content target anchor element when the skip to content link is clicked', () => { + const frame = mountWithAppProvider(); + const mainAnchor = frame.find('main').find('a'); + trigger(frame.find('a').at(0), 'onClick'); + expect(mainAnchor.getDOMNode()).toBe(document.activeElement); + }); - it('renders a TrapFocus with a `trapping` prop set to false prop around the navigation on small screens and showMobileNavigation is false', () => { - const navigation =
; - const frame = mountWithAppProvider(, { - mediaQuery: {isNavigationCollapsed: true}, - }).find(Frame); + it('sets focus to target element when the skip to content link is clicked', () => { + const targetId = 'SkipToContentTarget'; + const targetRef = React.createRef(); - const trapFocus = frame.find(TrapFocus); - expect(trapFocus.exists()).toBe(true); - expect(trapFocus.prop('trapping')).toBe(false); - }); + const skipToContentTarget = ( + // eslint-disable-next-line jsx-a11y/anchor-is-valid + + ); - it('renders a TrapFocus with a `trapping` prop set to false prop around the navigation on large screens even if showMobileNavigation is true', () => { - const navigation =
; - const trapFocus = mountWithAppProvider( - , - ).find(TrapFocus); + const frame = mountWithAppProvider( + {skipToContentTarget}, + ); - expect(trapFocus.exists()).toBe(true); - expect(trapFocus.prop('trapping')).toBe(false); - }); + const triggerAnchor = frame.find('a').at(0); + const targetAnchor = frame.find(`#${targetId}`); + trigger(triggerAnchor, 'onFocus'); + trigger(triggerAnchor, 'onClick'); - it('renders a CSSTransition around the navigation with `appear` and `exit` set to true on small screen', () => { - const navigation =
; - const cssTransition = mountWithAppProvider( - , - {mediaQuery: {isNavigationCollapsed: true}}, - ) - .find(TrapFocus) - .find(CSSTransition); - - expect(cssTransition.prop('appear')).toBe(true); - expect(cssTransition.prop('exit')).toBe(true); + expect(triggerAnchor.getDOMNode().getAttribute('href')).toBe( + `#${targetId}`, + ); + expect(targetAnchor.getDOMNode()).toBe(document.activeElement); + }); }); - it('renders a CSSTransition around the navigation with `appear` and `exit` set to false on large screen', () => { - const navigation =
; - const cssTransition = mountWithAppProvider( - , - ) - .find(TrapFocus) - .find(CSSTransition); + describe('topBar', () => { + it('renders with a top bar data attribute if a topBar is passed', () => { + const topbar =
; + const topBar = mountWithAppProvider(); - expect(cssTransition.prop('appear')).toBe(false); - expect(cssTransition.prop('exit')).toBe(false); - }); + expect(topBar.find('[data-polaris-top-bar]')).toHaveLength(1); + }); - it('renders a CSSTransition around the navigation with an `in` prop set to false if showMobileNavigation is true', () => { - const navigation =
; - const cssTransition = mountWithAppProvider( - , - ) - .find(Frame) - .find(TrapFocus) - .find(CSSTransition); + it('does not render with a top bar data attribute if none is passed', () => { + const topBar = mountWithAppProvider(); - expect(cssTransition.prop('in')).toBe(false); + expect(topBar.find('[data-polaris-top-bar]')).toHaveLength(0); + }); }); - it('renders a CSSTransition around the navigation with an `in` prop set to true if showMobileNavigation is true', () => { - const navigation =
; - const cssTransition = mountWithAppProvider( - , - ) - .find(Frame) - .find(TrapFocus) - .find(CSSTransition); + describe('navigation', () => { + it('renders a TrapFocus with a `trapping` prop set to true around the navigation on small screens and showMobileNavigation is true', () => { + const navigation =
; + const frame = mountWithAppProvider( + , + {mediaQuery: {isNavigationCollapsed: true}}, + ).find(Frame); - expect(cssTransition.prop('in')).toBe(true); - }); + const trapFocus = frame.find(TrapFocus); - it('renders a skip to content link with the proper text', () => { - const skipToContentLinkText = mountWithAppProvider() - .find('a') - .at(0) - .text(); + expect(trapFocus.exists()).toBe(true); + expect(trapFocus.prop('trapping')).toBe(true); + }); - expect(skipToContentLinkText).toStrictEqual('Skip to content'); - }); + it('renders a TrapFocus with a `trapping` prop set to false prop around the navigation on small screens and showMobileNavigation is false', () => { + const navigation =
; + const frame = mountWithAppProvider(, { + mediaQuery: {isNavigationCollapsed: true}, + }).find(Frame); - it('sets focus to the main content target anchor element when the skip to content link is clicked', () => { - const frame = mountWithAppProvider(); - const mainAnchor = frame.find('main').find('a'); - trigger(frame.find('a').at(0), 'onClick'); - expect(mainAnchor.getDOMNode()).toBe(document.activeElement); - }); + const trapFocus = frame.find(TrapFocus); + expect(trapFocus.exists()).toBe(true); + expect(trapFocus.prop('trapping')).toBe(false); + }); - it('sets focus to target element when the skip to content link is clicked', () => { - const targetId = 'SkipToContentTarget'; - const targetRef = React.createRef(); + it('renders a TrapFocus with a `trapping` prop set to false prop around the navigation on large screens even if showMobileNavigation is true', () => { + const navigation =
; + const trapFocus = mountWithAppProvider( + , + ).find(TrapFocus); - const skipToContentTarget = ( - // eslint-disable-next-line jsx-a11y/anchor-is-valid - - ); + expect(trapFocus.exists()).toBe(true); + expect(trapFocus.prop('trapping')).toBe(false); + }); - const frame = mountWithAppProvider( - {skipToContentTarget}, - ); + it('renders a CSSTransition around the navigation with `appear` and `exit` set to true on small screen', () => { + const navigation =
; + const cssTransition = mountWithAppProvider( + , + {mediaQuery: {isNavigationCollapsed: true}}, + ) + .find(TrapFocus) + .find(CSSTransition); + + expect(cssTransition.prop('appear')).toBe(true); + expect(cssTransition.prop('exit')).toBe(true); + }); - const triggerAnchor = frame.find('a').at(0); - const targetAnchor = frame.find(`#${targetId}`); - trigger(triggerAnchor, 'onFocus'); - trigger(triggerAnchor, 'onClick'); + it('renders a CSSTransition around the navigation with `appear` and `exit` set to false on large screen', () => { + const navigation =
; + const cssTransition = mountWithAppProvider( + , + ) + .find(TrapFocus) + .find(CSSTransition); - expect(triggerAnchor.getDOMNode().getAttribute('href')).toBe( - `#${targetId}`, - ); - expect(targetAnchor.getDOMNode()).toBe(document.activeElement); - }); + expect(cssTransition.prop('appear')).toBe(false); + expect(cssTransition.prop('exit')).toBe(false); + }); - it('renders with a has nav data attribute when nav is passed', () => { - const navigation =
; - const frame = mountWithAppProvider(); - expect(frame.find('[data-has-navigation]')).toHaveLength(1); - }); + it('renders a CSSTransition around the navigation with an `in` prop set to false if showMobileNavigation is true', () => { + const navigation =
; + const cssTransition = mountWithAppProvider( + , + ) + .find(Frame) + .find(TrapFocus) + .find(CSSTransition); - it('does not render with a has nav data attribute when nav is not passed', () => { - const frame = mountWithAppProvider(); - expect(frame.find('[data-has-navigation]')).toHaveLength(0); - }); + expect(cssTransition.prop('in')).toBe(false); + }); - it('renders with a top bar data attribute if a topBar is passed', () => { - const topbar =
; - const topBar = mountWithAppProvider(); + it('renders a CSSTransition around the navigation with an `in` prop set to true if showMobileNavigation is true', () => { + const navigation =
; + const cssTransition = mountWithAppProvider( + , + ) + .find(Frame) + .find(TrapFocus) + .find(CSSTransition); - expect(topBar.find('[data-polaris-top-bar]')).toHaveLength(1); - }); + expect(cssTransition.prop('in')).toBe(true); + }); + + it('renders with a has nav data attribute when nav is passed', () => { + const navigation =
; + const frame = mountWithAppProvider(); + expect(frame.find('[data-has-navigation]')).toHaveLength(1); + }); - it('does not render with a top bar data attribute if none is passed', () => { - const topBar = mountWithAppProvider(); + it('does not render with a has nav data attribute when nav is not passed', () => { + const frame = mountWithAppProvider(); + expect(frame.find('[data-has-navigation]')).toHaveLength(0); + }); - expect(topBar.find('[data-polaris-top-bar]')).toHaveLength(0); - }); + it('does not call onNavigationDismiss when escape is pressed and screen size is large', () => { + const spy = jest.fn(); + const frame = mountWithApp( + } onNavigationDismiss={spy} />, + ); + frame + .find('div', {id: 'AppFrameNav'})! + .trigger('onKeyDown', {key: 'Escape'}); - // JSDOM 11.12.0 does not support setting/reading custom properties so we are - // unable to assert that we set a custom property - // See https://github.com/jsdom/jsdom/issues/1895 - // eslint-disable-next-line jest/no-disabled-tests - it.skip('sets a root property with global ribbon height if passed', () => { - mountWithAppProvider(} />); - expect(documentHasStyle('--global-ribbon-height', '0px')).toBe(true); - }); + const {onNavigationDismiss} = frame.props as FrameProps; - // JSDOM 11.12.0 does not support setting/reading custom properties so we are - // unable to assert that we set a custom property - // See https://github.com/jsdom/jsdom/issues/1895 - // eslint-disable-next-line jest/no-disabled-tests - it.skip('sets a root property with global ribbon height if new props are passed', () => { - const frame = mountWithAppProvider(); - frame.setProps({globalRibbon:
}); - expect(documentHasStyle('--global-ribbon-height', '0px')).toBe(true); + expect(onNavigationDismiss).not.toHaveBeenCalled(); + }); + + it('calls onNavigationDismiss when escape is pressed and screen size is small', () => { + const spy = jest.fn(); + const frame = mountWithApp( + } + showMobileNavigation + onNavigationDismiss={spy} + />, + {mediaQuery: {isNavigationCollapsed: true}}, + ); + frame + .find('div', {id: 'AppFrameNav'})! + .trigger('onKeyDown', {key: 'Escape'}); + + const {onNavigationDismiss} = frame.props as FrameProps; + + expect(onNavigationDismiss).toHaveBeenCalledTimes(1); + }); }); - // JSDOM 11.12.0 does not support setting/reading custom properties so we are - // unable to assert that we set a custom property - // See https://github.com/jsdom/jsdom/issues/1895 - // eslint-disable-next-line jest/no-disabled-tests - it.skip('sets a root property with global ribbon height of 0 if there is no globalRibbon prop', () => { - mountWithAppProvider(); - expect(documentHasStyle('--global-ribbon-height', '0px')).toBe(true); + describe('globalRibbon', () => { + // JSDOM 11.12.0 does not support setting/reading custom properties so we are + // unable to assert that we set a custom property + // See https://github.com/jsdom/jsdom/issues/1895 + // eslint-disable-next-line jest/no-disabled-tests + it.skip('sets a root property with global ribbon height if passed', () => { + mountWithAppProvider(} />); + expect(documentHasStyle('--global-ribbon-height', '0px')).toBe(true); + }); + + // JSDOM 11.12.0 does not support setting/reading custom properties so we are + // unable to assert that we set a custom property + // See https://github.com/jsdom/jsdom/issues/1895 + // eslint-disable-next-line jest/no-disabled-tests + it.skip('sets a root property with global ribbon height if new props are passed', () => { + const frame = mountWithAppProvider(); + frame.setProps({globalRibbon:
}); + expect(documentHasStyle('--global-ribbon-height', '0px')).toBe(true); + }); + + // JSDOM 11.12.0 does not support setting/reading custom properties so we are + // unable to assert that we set a custom property + // See https://github.com/jsdom/jsdom/issues/1895 + // eslint-disable-next-line jest/no-disabled-tests + it.skip('sets a root property with global ribbon height of 0 if there is no globalRibbon prop', () => { + mountWithAppProvider(); + expect(documentHasStyle('--global-ribbon-height', '0px')).toBe(true); + }); }); - it('renders a Frame ContextualSavebar if Polaris ContextualSavebar is rendered', () => { - const frame = mountWithAppProvider( - - - , - ); - expect(frame.find(FrameContextualSavebar).exists()).toBe(true); + describe('ContextualSavebar', () => { + it('renders a Frame ContextualSavebar if Polaris ContextualSavebar is rendered', () => { + const frame = mountWithAppProvider( + + + , + ); + expect(frame.find(FrameContextualSavebar).exists()).toBe(true); + }); }); - it('renders a Frame Loading if Polaris Loading is rendered', () => { - const frame = mountWithAppProvider( - - - , - ); - expect(frame.find(FrameLoading).exists()).toBe(true); + describe('loading', () => { + it('renders a Frame Loading if Polaris Loading is rendered', () => { + const frame = mountWithAppProvider( + + + , + ); + expect(frame.find(FrameLoading).exists()).toBe(true); + }); }); }); From 7bf3d14a5b36c1063afa21a692e326c77203925e Mon Sep 17 00:00:00 2001 From: Kyle Durand Date: Wed, 8 Jan 2020 19:17:16 -0500 Subject: [PATCH 2/3] Update src/components/Frame/tests/Frame.test.tsx Co-Authored-By: Andrew Musgrave --- src/components/Frame/tests/Frame.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Frame/tests/Frame.test.tsx b/src/components/Frame/tests/Frame.test.tsx index 83c8410a913..e95fd040bc9 100644 --- a/src/components/Frame/tests/Frame.test.tsx +++ b/src/components/Frame/tests/Frame.test.tsx @@ -204,7 +204,7 @@ describe('', () => { const {onNavigationDismiss} = frame.props as FrameProps; - expect(onNavigationDismiss).not.toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); }); it('calls onNavigationDismiss when escape is pressed and screen size is small', () => { From e5a007728c9d03b6ac85124a40c1536380af22e3 Mon Sep 17 00:00:00 2001 From: Kyle Durand Date: Wed, 8 Jan 2020 19:17:24 -0500 Subject: [PATCH 3/3] Update src/components/Frame/tests/Frame.test.tsx Co-Authored-By: Andrew Musgrave --- src/components/Frame/tests/Frame.test.tsx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/components/Frame/tests/Frame.test.tsx b/src/components/Frame/tests/Frame.test.tsx index e95fd040bc9..fca5742314b 100644 --- a/src/components/Frame/tests/Frame.test.tsx +++ b/src/components/Frame/tests/Frame.test.tsx @@ -9,7 +9,7 @@ import { ContextualSaveBar as PolarisContextualSavebar, Loading as PolarisLoading, } from 'components'; -import Frame, {FrameProps} from '../Frame'; +import Frame from '../Frame'; import { ContextualSaveBar as FrameContextualSavebar, Loading as FrameLoading, @@ -202,8 +202,6 @@ describe('', () => { .find('div', {id: 'AppFrameNav'})! .trigger('onKeyDown', {key: 'Escape'}); - const {onNavigationDismiss} = frame.props as FrameProps; - expect(spy).not.toHaveBeenCalled(); }); @@ -221,9 +219,7 @@ describe('', () => { .find('div', {id: 'AppFrameNav'})! .trigger('onKeyDown', {key: 'Escape'}); - const {onNavigationDismiss} = frame.props as FrameProps; - - expect(onNavigationDismiss).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledTimes(1); }); });