From 82fb0402ff692cc6e0d3b41bfa215736375f330a Mon Sep 17 00:00:00 2001 From: Ben Scott Date: Fri, 30 Aug 2019 16:35:54 -0700 Subject: [PATCH] Hookify Avatar component Use react-testing in some Avatar tests as Enzyme doesn't like updating after hooky components after a setState --- UNRELEASED.md | 1 + src/components/Avatar/Avatar.tsx | 230 ++++++++---------- src/components/Avatar/index.ts | 4 +- .../Avatar/tests/Avatar-ssr.test.tsx | 3 +- src/components/Avatar/tests/Avatar.test.tsx | 44 ++-- tests/build.test.js | 2 +- 6 files changed, 134 insertions(+), 150 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index 6738509fde7..42ebfd33c25 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -42,6 +42,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Code quality - Removed mocks in various tests suites that are now redundant ([#1978](https://github.com/Shopify/polaris-react/pull/1978)) +- Migrated `Avatar` to use hooks instead of withAppProvider ([#2067](https://github.com/Shopify/polaris-react/pull/2067)) ### Deprecations diff --git a/src/components/Avatar/Avatar.tsx b/src/components/Avatar/Avatar.tsx index d7c4102bc7b..59a0d25ffce 100644 --- a/src/components/Avatar/Avatar.tsx +++ b/src/components/Avatar/Avatar.tsx @@ -1,11 +1,8 @@ -import React from 'react'; +import React, {useState, useCallback, useEffect} from 'react'; import {classNames, variationName} from '../../utilities/css'; +import {useI18n} from '../../utilities/i18n'; import {isServer} from '../../utilities/target'; -import { - withAppProvider, - WithAppProviderProps, -} from '../../utilities/with-app-provider'; import {Image} from '../Image'; import styles from './Avatar.scss'; @@ -13,6 +10,12 @@ import * as avatars from './images'; export type Size = 'small' | 'medium' | 'large'; +enum Status { + Pending = 'PENDING', + Loaded = 'LOADED', + Errored = 'ERRORED', +} + const STYLE_CLASSES = ['one', 'two', 'three', 'four', 'five', 'six']; const AVATAR_IMAGES = Object.keys(avatars).map( // import/namespace does not allow computed values by default @@ -38,127 +41,106 @@ export interface AvatarProps { accessibilityLabel?: string; } -interface State { - hasError: boolean; - hasLoaded: boolean; - prevSource?: string; -} - -type CombinedProps = AvatarProps & WithAppProviderProps; - -class Avatar extends React.PureComponent { - static getDerivedStateFromProps(props: AvatarProps, state: State) { - if (props.source !== state.prevSource) { - return { - prevSource: props.source, - hasError: false, - hasLoaded: false, - }; - } - - return null; +export function Avatar({ + name, + source, + initials, + customer, + size = 'medium', + accessibilityLabel, +}: AvatarProps) { + const i18n = useI18n(); + + const [status, setStatus] = useState(Status.Pending); + + // If the source changes, set the status back to pending + useEffect( + () => { + setStatus(Status.Pending); + }, + [source], + ); + + const handleError = useCallback(() => { + setStatus(Status.Errored); + }, []); + const handleLoad = useCallback(() => { + setStatus(Status.Loaded); + }, []); + + const hasImage = (source || customer) && status !== Status.Errored; + + const nameString = name || initials; + + let finalSource: string | undefined; + let label: string | undefined; + + if (accessibilityLabel) { + label = accessibilityLabel; + } else if (name) { + label = name; + } else if (initials) { + const splitInitials = initials.split('').join(' '); + label = i18n.translate('Polaris.Avatar.labelWithInitials', { + initials: splitInitials, + }); + } else { + label = i18n.translate('Polaris.Avatar.label'); } - state: State = { - hasError: false, - hasLoaded: false, - }; - - render() { - const { - name, - source, - initials, - customer, - size = 'medium', - accessibilityLabel, - polaris: {intl}, - } = this.props; - - const {hasError, hasLoaded} = this.state; - - const hasImage = (source || customer) && !hasError; - - const nameString = name || initials; - - let finalSource: string | undefined; - let label: string | undefined; - - if (accessibilityLabel) { - label = accessibilityLabel; - } else if (name) { - label = name; - } else if (initials) { - const splitInitials = initials.split('').join(' '); - label = intl.translate('Polaris.Avatar.labelWithInitials', { - initials: splitInitials, - }); - } else { - label = intl.translate('Polaris.Avatar.label'); - } - - if (source) { - finalSource = source; - } else if (customer) { - finalSource = customerPlaceholder(nameString); - } - - const className = classNames( - styles.Avatar, - styles[variationName('style', styleClass(nameString))], - size && styles[variationName('size', size)], - hasImage && !hasLoaded && styles.hidden, - hasImage && styles.hasImage, - ); - - const imageMarkUp = - finalSource && !isServer && !hasError ? ( - - ) : null; - - // Use `dominant-baseline: central` instead of `dy` when Edge supports it. - const verticalOffset = '0.35em'; - - const initialsMarkup = - initials && !hasImage ? ( - - - - {initials} - - - - ) : null; - - return ( - - {initialsMarkup} - {imageMarkUp} - - ); + if (source) { + finalSource = source; + } else if (customer) { + finalSource = customerPlaceholder(nameString); } - handleError = () => { - this.setState({hasError: true, hasLoaded: false}); - }; - - handleLoad = () => { - this.setState({hasLoaded: true, hasError: false}); - }; + const className = classNames( + styles.Avatar, + styles[variationName('style', styleClass(nameString))], + size && styles[variationName('size', size)], + hasImage && status !== Status.Loaded && styles.hidden, + hasImage && styles.hasImage, + ); + + const imageMarkUp = + finalSource && !isServer && status !== Status.Errored ? ( + + ) : null; + + // Use `dominant-baseline: central` instead of `dy` when Edge supports it. + const verticalOffset = '0.35em'; + + const initialsMarkup = + initials && !hasImage ? ( + + + + {initials} + + + + ) : null; + + return ( + + {initialsMarkup} + {imageMarkUp} + + ); } function styleClass(name?: string) { @@ -172,7 +154,3 @@ function customerPlaceholder(name?: string) { ? AVATAR_IMAGES[name.charCodeAt(0) % AVATAR_IMAGES.length] : AVATAR_IMAGES[0]; } - -// Use named export once withAppProvider is refactored away -// eslint-disable-next-line import/no-default-export -export default withAppProvider()(Avatar); diff --git a/src/components/Avatar/index.ts b/src/components/Avatar/index.ts index 07d30c58cdf..2822843f0cf 100644 --- a/src/components/Avatar/index.ts +++ b/src/components/Avatar/index.ts @@ -1,3 +1 @@ -import Avatar, {AvatarProps} from './Avatar'; - -export {Avatar, AvatarProps}; +export {Avatar, AvatarProps} from './Avatar'; diff --git a/src/components/Avatar/tests/Avatar-ssr.test.tsx b/src/components/Avatar/tests/Avatar-ssr.test.tsx index e9e1b2898bb..47650bf441d 100644 --- a/src/components/Avatar/tests/Avatar-ssr.test.tsx +++ b/src/components/Avatar/tests/Avatar-ssr.test.tsx @@ -1,7 +1,6 @@ import React from 'react'; import {mountWithAppProvider} from 'test-utilities/legacy'; -import {Image} from 'components'; -import Avatar from '../Avatar'; +import {Avatar, Image} from 'components'; jest.mock('../../../utilities/target', () => ({ get isServer() { diff --git a/src/components/Avatar/tests/Avatar.test.tsx b/src/components/Avatar/tests/Avatar.test.tsx index f81454771a9..23edd8f8c42 100644 --- a/src/components/Avatar/tests/Avatar.test.tsx +++ b/src/components/Avatar/tests/Avatar.test.tsx @@ -1,7 +1,7 @@ import React from 'react'; -import {mountWithAppProvider, trigger} from 'test-utilities/legacy'; -import {Image} from 'components'; -import Avatar from '../Avatar'; +import {mountWithAppProvider} from 'test-utilities/legacy'; +import {mountWithApp} from 'test-utilities'; +import {Avatar, Image} from 'components'; describe('', () => { describe('intials', () => { @@ -45,36 +45,44 @@ describe('', () => { describe('on Error with Initials', () => { it('renders initials if the Image onError prop is triggered and the Intials are provided', () => { - const src = 'image/path/'; - const avatar = mountWithAppProvider( - , + const avatar = mountWithApp( + , ); - expect(avatar.find('span[role="img"] span svg')).toHaveLength(0); - trigger(avatar.find(Image), 'onError'); - expect(avatar.find('span[role="img"] span svg')).toHaveLength(1); + + expect(avatar).toContainReactComponent(Image); + expect(avatar).not.toContainReactComponent('span', { + className: 'Initials', + }); + + avatar.find(Image)!.trigger('onError'); + + expect(avatar).not.toContainReactComponent(Image); + expect(avatar).toContainReactComponent('span', { + className: 'Initials', + }); }); }); describe('on Error with changed props', () => { it('re-renders the image if a the source prop is changed after an error', () => { - const src = 'image/path/'; const workingSrc = 'image/goodPath/'; - const avatar = mountWithAppProvider( - , + const avatar = mountWithApp( + , ); - trigger(avatar.find(Image), 'onError'); - expect(avatar.find(Image)).toHaveLength(0); + avatar.find(Image)!.trigger('onError'); + + expect(avatar).not.toContainReactComponent(Image); + avatar.setProps({source: workingSrc}); - const image = avatar.find(Image); - expect(image).toHaveLength(1); + expect(avatar).toContainReactComponent(Image); }); }); describe('on Load', () => { it('safely triggers onLoad', () => { - const avatar = mountWithAppProvider(); + const avatar = mountWithApp(); expect(() => { - trigger(avatar.find(Image), 'onLoad'); + avatar.find(Image)!.trigger('onLoad'); }).not.toThrow(); }); }); diff --git a/tests/build.test.js b/tests/build.test.js index edb714f2b78..1397440df09 100644 --- a/tests/build.test.js +++ b/tests/build.test.js @@ -134,7 +134,7 @@ describe('build', () => { 'esnext/components/Avatar/index.js', 'utf8', ); - expect(contents).toMatch("import Avatar from './Avatar'"); + expect(contents).toMatch("export { Avatar } from './Avatar'"); }); it('preserves ES scss imports', () => {