From fef640fb4640c76b6ebd38403c8a532613327e76 Mon Sep 17 00:00:00 2001 From: Tom Bonnike Date: Wed, 26 Aug 2020 11:38:50 +0200 Subject: [PATCH 1/3] Refactor Banner to a functional component --- src/components/Banner/Banner.tsx | 397 +++++++++++++++++-------------- 1 file changed, 212 insertions(+), 185 deletions(-) diff --git a/src/components/Banner/Banner.tsx b/src/components/Banner/Banner.tsx index 22fc055b2e3..8fde77c905b 100644 --- a/src/components/Banner/Banner.tsx +++ b/src/components/Banner/Banner.tsx @@ -1,4 +1,10 @@ -import React, {PureComponent, createRef} from 'react'; +import React, { + forwardRef, + useRef, + useState, + useContext, + useImperativeHandle, +} from 'react'; import { CancelSmallMinor, CircleTickMajorTwotone, @@ -12,9 +18,9 @@ import { CircleDisabledMajorFilled, } from '@shopify/polaris-icons'; -import {FeaturesContext} from '../../utilities/features'; import {BannerContext} from '../../utilities/banner-context'; import {classNames, variationName} from '../../utilities/css'; +import {useFeatures} from '../../utilities/features'; import type { Action, DisableableAction, @@ -32,10 +38,6 @@ import styles from './Banner.scss'; export type BannerStatus = 'success' | 'info' | 'warning' | 'critical'; -interface State { - showFocus: boolean; -} - export interface BannerProps { /** Title content for the banner. */ title?: string; @@ -55,202 +57,133 @@ export interface BannerProps { stopAnnouncements?: boolean; } -export class Banner extends PureComponent { - static contextType = FeaturesContext; - context!: React.ContextType; - - state: State = { - showFocus: false, - }; +export const Banner = forwardRef(function Banner( + { + icon, + action, + secondaryAction, + title, + children, + status, + onDismiss, + stopAnnouncements, + }: BannerProps, + bannerRef, +) { + const {newDesignLanguage} = useFeatures(); + const withinContentContainer = useContext(WithinContentContext); + const buttonSizeValue = withinContentContainer ? 'slim' : undefined; + const id = uniqueID(); + const { + wrapperRef, + handleKeyUp, + handleBlur, + handleMouseUp, + shouldShowFocus, + } = useBannerFocus(bannerRef); + const {defaultIcon, iconColor, ariaRoleType} = useBannerAttributes( + status, + newDesignLanguage, + ); + const iconName = icon || defaultIcon; + const className = classNames( + styles.Banner, + status && styles[variationName('status', status)], + onDismiss && styles.hasDismiss, + shouldShowFocus && styles.keyFocused, + withinContentContainer ? styles.withinContentContainer : styles.withinPage, + newDesignLanguage && styles.newDesignLanguage, + ); - private wrapper = createRef(); + let headingMarkup: React.ReactNode = null; + let headingID: string | undefined; - public focus() { - this.wrapper.current && this.wrapper.current.focus(); - this.setState({showFocus: true}); + if (title) { + headingID = `${id}Heading`; + headingMarkup = ( +
+ {title} +
+ ); } - render() { - const {newDesignLanguage} = this.context || {}; - const {showFocus} = this.state; - - const handleKeyUp = (evt: React.KeyboardEvent) => { - if (evt.target === this.wrapper.current) { - this.setState({showFocus: true}); - } - }; - - const handleBlur = () => { - this.setState({showFocus: false}); - }; - - const handleMouseUp = ({ - currentTarget, - }: React.MouseEvent) => { - const {showFocus} = this.state; - currentTarget.blur(); - showFocus && this.setState({showFocus: false}); - }; - - return ( - - - {(withinContentContainer) => { - const { - icon, - action, - secondaryAction, - title, - children, - status, - onDismiss, - stopAnnouncements, - } = this.props; - let color: IconProps['color']; - let defaultIcon: IconProps['source']; - let ariaRoleType = 'status'; - - switch (status) { - case 'success': - color = newDesignLanguage ? 'success' : 'greenDark'; - defaultIcon = newDesignLanguage - ? CircleTickMajorFilled - : CircleTickMajorTwotone; - break; - case 'info': - color = newDesignLanguage ? 'highlight' : 'tealDark'; - defaultIcon = newDesignLanguage - ? CircleInformationMajorFilled - : CircleInformationMajorTwotone; - break; - case 'warning': - color = newDesignLanguage ? 'warning' : 'yellowDark'; - defaultIcon = newDesignLanguage - ? CircleAlertMajorFilled - : CircleAlertMajorTwotone; - ariaRoleType = 'alert'; - break; - case 'critical': - color = newDesignLanguage ? 'critical' : 'redDark'; - defaultIcon = newDesignLanguage - ? CircleDisabledMajorFilled - : CircleDisabledMajorTwotone; - ariaRoleType = 'alert'; - break; - default: - color = newDesignLanguage ? 'base' : 'inkLighter'; - defaultIcon = newDesignLanguage - ? CircleInformationMajorFilled - : FlagMajorTwotone; - } - const className = classNames( - styles.Banner, - status && styles[variationName('status', status)], - onDismiss && styles.hasDismiss, - showFocus && styles.keyFocused, - withinContentContainer - ? styles.withinContentContainer - : styles.withinPage, - newDesignLanguage && styles.newDesignLanguage, - ); - - const id = uniqueID(); - const iconName = icon || defaultIcon; - - let headingMarkup: React.ReactNode = null; - let headingID: string | undefined; + const actionMarkup = action && ( +
+ +
+ {buttonFrom(action, {outline: true, size: buttonSizeValue})} +
- if (title) { - headingID = `${id}Heading`; - headingMarkup = ( -
- {title} -
- ); - } - - const buttonSizeValue = withinContentContainer ? 'slim' : undefined; + {secondaryAction && } + +
+ ); - const secondaryActionMarkup = secondaryAction - ? secondaryActionFrom(secondaryAction) - : null; + let contentMarkup: React.ReactNode = null; + let contentID: string | undefined; - const actionMarkup = action ? ( -
- -
- {buttonFrom(action, {outline: true, size: buttonSizeValue})} -
- {secondaryActionMarkup} - -
- ) : null; + if (children || actionMarkup) { + contentID = `${id}Content`; + contentMarkup = ( +
+ {children} + {actionMarkup} +
+ ); + } - let contentMarkup = null; - let contentID: string | undefined; + const dismissButton = onDismiss && ( +
+
+ ); - if (children || actionMarkup) { - contentID = `${id}Content`; - contentMarkup = ( -
- {children} - {actionMarkup} -
- ); - } + return ( + +
+ {dismissButton} - const dismissButton = onDismiss ? ( -
-
- ) : null; +
+ +
- return ( -
- {dismissButton} -
- -
-
- {headingMarkup} - {contentMarkup} -
-
- ); - }} - - - ); - } -} +
+ {headingMarkup} + {contentMarkup} +
+
+ + ); +}); let index = 1; + function uniqueID() { return `Banner${index++}`; } -function secondaryActionFrom(action: Action) { +function SecondaryActionFrom({action}: {action: Action}) { if (action.url) { return ( ); } + +interface BannerAttributes { + iconColor: IconProps['color']; + defaultIcon: IconProps['source']; + ariaRoleType: 'status' | 'alert'; +} + +function useBannerAttributes( + status: BannerProps['status'], + newDesignLanguage: boolean, +): BannerAttributes { + switch (status) { + case 'success': + return { + defaultIcon: newDesignLanguage + ? CircleTickMajorFilled + : CircleTickMajorTwotone, + iconColor: newDesignLanguage ? 'success' : 'greenDark', + ariaRoleType: 'status', + }; + + case 'info': + return { + defaultIcon: newDesignLanguage + ? CircleInformationMajorFilled + : CircleInformationMajorTwotone, + iconColor: newDesignLanguage ? 'highlight' : 'tealDark', + ariaRoleType: 'status', + }; + + case 'warning': + return { + defaultIcon: newDesignLanguage + ? CircleAlertMajorFilled + : CircleAlertMajorTwotone, + iconColor: newDesignLanguage ? 'warning' : 'yellowDark', + ariaRoleType: 'alert', + }; + + case 'critical': + return { + defaultIcon: newDesignLanguage + ? CircleDisabledMajorFilled + : CircleDisabledMajorTwotone, + iconColor: newDesignLanguage ? 'critical' : 'redDark', + ariaRoleType: 'alert', + }; + + default: + return { + defaultIcon: newDesignLanguage + ? CircleInformationMajorFilled + : FlagMajorTwotone, + iconColor: newDesignLanguage ? 'base' : 'inkLighter', + ariaRoleType: 'status', + }; + } +} + +export interface BannerHandles { + focus(): void; +} + +function useBannerFocus(bannerRef: React.Ref) { + const wrapperRef = useRef(null); + const [shouldShowFocus, setShouldShowFocus] = useState(false); + + useImperativeHandle(bannerRef, () => ({ + focus: () => { + wrapperRef.current?.focus(); + setShouldShowFocus(true); + }, + })); + + const handleKeyUp = (event: React.KeyboardEvent) => { + if (event.target === wrapperRef.current) { + setShouldShowFocus(true); + } + }; + + const handleBlur = () => setShouldShowFocus(false); + const handleMouseUp = (event: React.MouseEvent) => { + event.currentTarget.blur(); + setShouldShowFocus(false); + }; + + return { + wrapperRef, + handleKeyUp, + handleBlur, + handleMouseUp, + shouldShowFocus, + }; +} From b2a95591c6732738d015d88a69a2d680ee2b1805 Mon Sep 17 00:00:00 2001 From: Tom Bonnike Date: Wed, 26 Aug 2020 14:08:41 +0200 Subject: [PATCH 2/3] =?UTF-8?q?Fix=20Banner=20id=E2=80=99s=20server/client?= =?UTF-8?q?=20mismatch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using the `useUniqueId` hook instead of the custom `uniqueId` util --- UNRELEASED.md | 1 + src/components/Banner/Banner.tsx | 11 +++-------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index 14958a702a4..04fd5101100 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -15,6 +15,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Bug fixes - Fix `Button` css in a `connectedTop` or `fullWidth` `ButtonGroup` ([#3215](https://github.com/Shopify/polaris-react/pull/3215)). +- Fixed `Banner`’s `id` being mismatched on server VS client ([#3199](https://github.com/Shopify/polaris-react/pull/3199)). ### Documentation diff --git a/src/components/Banner/Banner.tsx b/src/components/Banner/Banner.tsx index 8fde77c905b..be7da08476e 100644 --- a/src/components/Banner/Banner.tsx +++ b/src/components/Banner/Banner.tsx @@ -18,9 +18,10 @@ import { CircleDisabledMajorFilled, } from '@shopify/polaris-icons'; -import {BannerContext} from '../../utilities/banner-context'; import {classNames, variationName} from '../../utilities/css'; +import {BannerContext} from '../../utilities/banner-context'; import {useFeatures} from '../../utilities/features'; +import {useUniqueId} from '../../utilities/unique-id'; import type { Action, DisableableAction, @@ -73,7 +74,7 @@ export const Banner = forwardRef(function Banner( const {newDesignLanguage} = useFeatures(); const withinContentContainer = useContext(WithinContentContext); const buttonSizeValue = withinContentContainer ? 'slim' : undefined; - const id = uniqueID(); + const id = useUniqueId('Banner'); const { wrapperRef, handleKeyUp, @@ -177,12 +178,6 @@ export const Banner = forwardRef(function Banner( ); }); -let index = 1; - -function uniqueID() { - return `Banner${index++}`; -} - function SecondaryActionFrom({action}: {action: Action}) { if (action.url) { return ( From fb75003c5ba0566a2b2a547f3d6666595e67e616 Mon Sep 17 00:00:00 2001 From: Tom Bonnike Date: Fri, 4 Sep 2020 10:50:36 +0200 Subject: [PATCH 3/3] Add tests for the `role` attribute in the Banner component --- src/components/Banner/tests/Banner.test.tsx | 35 ++++++++++++--------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/components/Banner/tests/Banner.test.tsx b/src/components/Banner/tests/Banner.test.tsx index 7b6b10f85df..3bb1872d26c 100644 --- a/src/components/Banner/tests/Banner.test.tsx +++ b/src/components/Banner/tests/Banner.test.tsx @@ -36,24 +36,41 @@ describe('', () => { expect(banner.find(Icon).prop('source')).toBe(CirclePlusMinor); }); - it('uses a greenDark circleCheckMark if status is success', () => { + it('uses a greenDark circleCheckMark if status is success and sets a status aria role', () => { const banner = mountWithAppProvider(); expect(banner.find(Icon).prop('source')).toBe(CircleTickMajorTwotone); expect(banner.find(Icon).prop('color')).toBe('greenDark'); + expect(banner.find('div').first().prop('role')).toBe('status'); }); - it('uses a tealDark circleInformation if status is info', () => { + it('uses a tealDark circleInformation if status is info and sets a status aria role', () => { const banner = mountWithAppProvider(); expect(banner.find(Icon).prop('source')).toBe( CircleInformationMajorTwotone, ); expect(banner.find(Icon).prop('color')).toBe('tealDark'); + expect(banner.find('div').first().prop('role')).toBe('status'); }); - it('uses a yellowDark circleAlert if status is warning', () => { + it('uses a yellowDark circleAlert if status is warning and sets an alert aria role', () => { const banner = mountWithAppProvider(); expect(banner.find(Icon).prop('source')).toBe(CircleAlertMajorTwotone); expect(banner.find(Icon).prop('color')).toBe('yellowDark'); + expect(banner.find('div').first().prop('role')).toBe('alert'); + }); + + it('uses a redDark circleBarred if status is critical and sets an alert aria role', () => { + const banner = mountWithAppProvider(); + expect(banner.find(Icon).prop('source')).toBe(CircleDisabledMajorTwotone); + expect(banner.find(Icon).prop('color')).toBe('redDark'); + expect(banner.find('div').first().prop('role')).toBe('alert'); + }); + + it('uses a default icon and aria role', () => { + const banner = mountWithAppProvider(); + expect(banner.find(Icon).prop('source')).toBe(FlagMajorTwotone); + expect(banner.find(Icon).prop('color')).toBe('inkLighter'); + expect(banner.find('div').first().prop('role')).toBe('status'); }); it('disables aria-live when stopAnnouncements is enabled', () => { @@ -68,18 +85,6 @@ describe('', () => { expect(quietBanner).toBeTruthy(); }); - it('uses a redDark circleBarred if status is critical', () => { - const banner = mountWithAppProvider(); - expect(banner.find(Icon).prop('source')).toBe(CircleDisabledMajorTwotone); - expect(banner.find(Icon).prop('color')).toBe('redDark'); - }); - - it('uses a default icon', () => { - const banner = mountWithAppProvider(); - expect(banner.find(Icon).prop('source')).toBe(FlagMajorTwotone); - expect(banner.find(Icon).prop('color')).toBe('inkLighter'); - }); - describe('onDismiss()', () => { it('is called when the dismiss button is clicked', () => { const spy = jest.fn();