From d5bad95853b9220d3a93a99cfc9a3a746f6aa3ea Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Mon, 16 May 2022 18:56:02 -0500 Subject: [PATCH 1/7] test(fp-1650): add test cases, add prop value maps --- .../src/components/_common/Button/Button.jsx | 51 +++--- .../components/_common/Button/Button.test.js | 164 ++++++++++++++++-- client/src/components/_common/Icon/Icon.jsx | 12 +- 3 files changed, 186 insertions(+), 41 deletions(-) diff --git a/client/src/components/_common/Button/Button.jsx b/client/src/components/_common/Button/Button.jsx index 146620800..28caf8dcd 100644 --- a/client/src/components/_common/Button/Button.jsx +++ b/client/src/components/_common/Button/Button.jsx @@ -5,9 +5,20 @@ import Icon from '../Icon'; import styles from './Button.module.css'; import LoadingSpinner from '_common/LoadingSpinner'; -export const TYPES = ['', 'primary', 'secondary', 'link']; +export const TYPE_MAP = { + primary: 'primary', + secondary: 'secondary', + link: 'as-link', +}; +export const TYPES = [''].concat(Object.keys(TYPE_MAP)); -export const SIZES = ['', 'short', 'medium', 'long', 'small']; +export const SIZE_MAP = { + short: 'width-short', + medium: 'width-medium', + long: 'width-long', + small: 'size-small', +}; +export const SIZES = [''].concat(Object.keys(SIZE_MAP)); export const ATTRIBUTES = ['button', 'submit', 'reset']; @@ -24,6 +35,7 @@ const Button = ({ iconNameAfter, type, size, + dataTestid, disabled, onClick, attr, @@ -64,37 +76,18 @@ const Button = ({ } /* eslint-enable no-console */ - const buttonRootClass = styles['root']; - - let buttonTypeClass; - if (type === 'link') { - buttonTypeClass = styles['as-link']; - } else if (type === 'primary' || type === 'secondary') { - buttonTypeClass = styles[`${type}`]; - } else if (type === '') { - buttonTypeClass = type; - } - - let buttonSizeClass; - if (size === 'small') { - buttonSizeClass = styles['size-small']; - } else { - buttonSizeClass = styles[`width-${size}`]; - } - - const buttonLoadingClass = isLoading ? styles['loading'] : ''; - return ( + it('uses given text', () => { + muteTypeNotLinkNoSizeLog(); + const { getByTestId } = render( + ); - const el = queryByTestId('no button here'); - expect(el).toBeNull; + expect(getByTestId('text').textContent).toEqual(TEST_TEXT); }); - it('disables button when in loading state', () => { - const { queryByText } = render( - - ); - const el = queryByText('Loading Button'); - expect(el).toBeDisabled; + + describe('icons exist as expected when', () => { + test('only `iconNameBefore` is given', () => { + muteTypeNotLinkNoSizeLog(); + const { queryByTestId } = render( + + ); + expect(queryByTestId('icon-before')).toBeInTheDocument(); + expect(queryByTestId('icon-after')).not.toBeInTheDocument(); + }); + test('only `iconNameAfter` is given', () => { + muteTypeNotLinkNoSizeLog(); + const { queryByTestId } = render( + + ); + expect(queryByTestId('icon-before')).not.toBeInTheDocument(); + expect(queryByTestId('icon-after')).toBeInTheDocument(); + }); + test('both `iconNameAfter` and `iconNameBefore` are given', () => { + muteTypeNotLinkNoSizeLog(); + const { queryByTestId } = render( + + ); + expect(queryByTestId('icon-before')).toBeInTheDocument(); + expect(queryByTestId('icon-after')).toBeInTheDocument(); + }); + }); + + describe('all type & size combinations render accurately', () => { + it.each(BTN.TYPES)('type is "%s"', (type) => { + muteTypeNotLinkNoSizeLog(); + if (isPropertyLimitation(type, TEST_SIZE)) { + return Promise.resolve(); + } + const { getByRole, getByTestId } = render( + + ); + + testClassnamesByType(type, TEST_SIZE, getByRole, getByTestId); + }); + it.each(BTN.SIZES)('size is "%s"', (size) => { + muteTypeNotLinkNoSizeLog(); + if (isPropertyLimitation(TEST_TYPE, size)) { + return Promise.resolve(); + } + const { getByRole, getByTestId } = render( + + ); + + testClassnamesByType(TEST_TYPE, size, getByRole, getByTestId); + }); + }); + + describe('loading', () => { + it('does not render button without text', () => { + muteTypeNotLinkNoSizeLog(); + const { queryByTestId } = render( + + ); + const el = queryByTestId('no button here'); + expect(el).toBeNull; + }); + it('disables button when in loading state', () => { + muteTypeNotLinkNoSizeLog(); + const { queryByText } = render( + + ); + const el = queryByText('Loading Button'); + expect(el).toBeDisabled; + }); + }); + + describe('property limitation', () => { + test('type is "link" & ANY size`', () => { + console.warn = jest.fn(); + const { getByRole, getByTestId } = render( + + ); + const expectedType = 'link'; + const expectedSize = ''; + + testClassnamesByType(expectedType, expectedSize, getByRole, getByTestId); + expect(console.warn).toHaveBeenCalled(); + }); + test('type is "primary" & size is "small"', () => { + console.error = jest.fn(); + const { getByRole, getByTestId } = render( + + ); + const expectedType = 'secondary'; + const expectedSize = 'small'; + + testClassnamesByType(expectedType, expectedSize, getByRole, getByTestId); + expect(console.error).toHaveBeenCalled(); + }); + test('type is not "link" & NO size`', () => { + console.debug = jest.fn(); + const { getByRole, getByTestId } = render( + + ); + const expectedType = 'primary'; + const expectedSize = 'short'; + + testClassnamesByType(expectedType, expectedSize, getByRole, getByTestId); + expect(console.debug).toHaveBeenCalled(); + }); }); }); diff --git a/client/src/components/_common/Icon/Icon.jsx b/client/src/components/_common/Icon/Icon.jsx index 657fd01ee..d0aa85171 100644 --- a/client/src/components/_common/Icon/Icon.jsx +++ b/client/src/components/_common/Icon/Icon.jsx @@ -2,7 +2,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import './Icon.module.css'; -const Icon = ({ children, className, name }) => { +const Icon = ({ children, className, dataTestid, name }) => { const iconClassName = `icon icon-${name}`; // FAQ: The conditional avoids an extra space in class attribute value const fullClassName = className @@ -10,19 +10,27 @@ const Icon = ({ children, className, name }) => { : iconClassName; const label = children; - return ; + return ; }; Icon.propTypes = { /** A text alternative to the icon (for accessibility) */ children: PropTypes.string, /** Additional className for the root element */ className: PropTypes.string, + /** ID for test case element selection */ + dataTestid: PropTypes.string, /** Name of icon from icon font (without the (`icon-` prefix) */ name: PropTypes.string.isRequired, }; Icon.defaultProps = { children: '', className: '', + dataTestid: '', }; export default Icon; From 7499aca048233748371629a5d7d4831d75648461 Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Mon, 16 May 2022 19:15:02 -0500 Subject: [PATCH 2/7] chore(fp-1650): prettier fix --- .../src/components/_common/Button/Button.jsx | 4 +- .../components/_common/Button/Button.test.js | 37 ++++++++++++------- client/src/components/_common/Icon/Icon.jsx | 14 ++++--- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/client/src/components/_common/Button/Button.jsx b/client/src/components/_common/Button/Button.jsx index 28caf8dcd..275858cef 100644 --- a/client/src/components/_common/Button/Button.jsx +++ b/client/src/components/_common/Button/Button.jsx @@ -104,7 +104,9 @@ const Button = ({ ) : ( '' )} - {children} + + {children} + {iconNameAfter && ( { it('uses given text', () => { muteTypeNotLinkNoSizeLog(); - const { getByTestId } = render( - - ); + const { getByTestId } = render(); expect(getByTestId('text').textContent).toEqual(TEST_TEXT); }); @@ -71,7 +72,9 @@ describe('Button', () => { test('both `iconNameAfter` and `iconNameBefore` are given', () => { muteTypeNotLinkNoSizeLog(); const { queryByTestId } = render( - + ); expect(queryByTestId('icon-before')).toBeInTheDocument(); expect(queryByTestId('icon-after')).toBeInTheDocument(); @@ -85,7 +88,9 @@ describe('Button', () => { return Promise.resolve(); } const { getByRole, getByTestId } = render( - + ); testClassnamesByType(type, TEST_SIZE, getByRole, getByTestId); @@ -96,7 +101,9 @@ describe('Button', () => { return Promise.resolve(); } const { getByRole, getByTestId } = render( - + ); testClassnamesByType(TEST_TYPE, size, getByRole, getByTestId); @@ -126,7 +133,9 @@ describe('Button', () => { test('type is "link" & ANY size`', () => { console.warn = jest.fn(); const { getByRole, getByTestId } = render( - + ); const expectedType = 'link'; const expectedSize = ''; @@ -137,7 +146,9 @@ describe('Button', () => { test('type is "primary" & size is "small"', () => { console.error = jest.fn(); const { getByRole, getByTestId } = render( - + ); const expectedType = 'secondary'; const expectedSize = 'small'; diff --git a/client/src/components/_common/Icon/Icon.jsx b/client/src/components/_common/Icon/Icon.jsx index d0aa85171..a532e3bec 100644 --- a/client/src/components/_common/Icon/Icon.jsx +++ b/client/src/components/_common/Icon/Icon.jsx @@ -10,12 +10,14 @@ const Icon = ({ children, className, dataTestid, name }) => { : iconClassName; const label = children; - return ; + return ( + + ); }; Icon.propTypes = { /** A text alternative to the icon (for accessibility) */ From 38af56b72efbabba4490c301a24d6db0716b497f Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Thu, 2 Jun 2022 18:13:13 -0500 Subject: [PATCH 3/7] fix(fp-1650): don't fail on empty className Button does not need to have a class. This msitake came form me in #631. --- client/src/components/_common/Button/Button.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/components/_common/Button/Button.jsx b/client/src/components/_common/Button/Button.jsx index cf3026a79..6a20bdcf9 100644 --- a/client/src/components/_common/Button/Button.jsx +++ b/client/src/components/_common/Button/Button.jsx @@ -122,7 +122,7 @@ const Button = ({ }; Button.propTypes = { children: isNotEmptyString, - className: isNotEmptyString, + className: PropTypes.string, iconNameBefore: PropTypes.string, iconNameAfter: PropTypes.string, type: PropTypes.oneOf(TYPES), From a6c81cd3cfd0dd39906e03bb7d43a8e7cddc9241 Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Mon, 13 Jun 2022 18:14:21 -0500 Subject: [PATCH 4/7] =?UTF-8?q?fix(fp-1650):=20dataTestid=20default,=20''?= =?UTF-8?q?=20=E2=86=92=20undefined?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also changed instances in two other components. --- client/src/components/_common/Button/Button.jsx | 2 +- client/src/components/_common/Icon/Icon.jsx | 2 +- client/src/components/_common/Message/Message.jsx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/src/components/_common/Button/Button.jsx b/client/src/components/_common/Button/Button.jsx index 6a20bdcf9..2af807c11 100644 --- a/client/src/components/_common/Button/Button.jsx +++ b/client/src/components/_common/Button/Button.jsx @@ -139,7 +139,7 @@ Button.defaultProps = { iconNameAfter: '', type: 'secondary', size: '', // unless `type="link", defaults to `short` after `propTypes` - dataTestid: '', + dataTestid: undefined, disabled: false, onClick: null, attr: 'button', diff --git a/client/src/components/_common/Icon/Icon.jsx b/client/src/components/_common/Icon/Icon.jsx index a532e3bec..70ce4bca5 100644 --- a/client/src/components/_common/Icon/Icon.jsx +++ b/client/src/components/_common/Icon/Icon.jsx @@ -32,7 +32,7 @@ Icon.propTypes = { Icon.defaultProps = { children: '', className: '', - dataTestid: '', + dataTestid: undefined, }; export default Icon; diff --git a/client/src/components/_common/Message/Message.jsx b/client/src/components/_common/Message/Message.jsx index 8344f9ffe..256da4fb6 100644 --- a/client/src/components/_common/Message/Message.jsx +++ b/client/src/components/_common/Message/Message.jsx @@ -194,7 +194,7 @@ Message.defaultProps = { ariaLabel: 'message', className: '', canDismiss: false, - dataTestid: '', + dataTestid: undefined, isVisible: true, onDismiss: () => {}, scope: '', // RFE: Require scope; remove this line From 17049b009709db4688c963f15fb8c7893c7fe6f8 Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Mon, 13 Jun 2022 18:23:17 -0500 Subject: [PATCH 5/7] fix(fp-1650): remove unused function --- client/src/components/_common/Button/Button.test.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/client/src/components/_common/Button/Button.test.js b/client/src/components/_common/Button/Button.test.js index 9edffdbb1..eda303e6c 100644 --- a/client/src/components/_common/Button/Button.test.js +++ b/client/src/components/_common/Button/Button.test.js @@ -37,14 +37,6 @@ function isPropertyLimitation(type, size) { return isLimited; } -function getExpectedType(type, size) { - let expType = type; - if (type === 'primary' && size === 'small') { - expType = ''; - } - return expType; -} - describe('Button', () => { it('uses given text', () => { muteTypeNotLinkNoSizeLog(); From e80f9de1af9160ff8f73bfe906271957b0908d69 Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Mon, 13 Jun 2022 18:24:09 -0500 Subject: [PATCH 6/7] fix(fp-1650): fixes from tup-ui PR Source: https://github.com/TACC/tup-ui/pull/7/files#diff-7c7d6d82c4485c00f402e951eb76d77bfad3a2f3b567fa9f2330eb47a85f814a --- client/src/components/_common/Button/Button.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/src/components/_common/Button/Button.test.js b/client/src/components/_common/Button/Button.test.js index eda303e6c..ad4021af4 100644 --- a/client/src/components/_common/Button/Button.test.js +++ b/client/src/components/_common/Button/Button.test.js @@ -109,7 +109,7 @@ describe('Button', () => { ); const el = queryByTestId('no button here'); - expect(el).toBeNull; + expect(el).toBeNull(); }); it('disables button when in loading state', () => { muteTypeNotLinkNoSizeLog(); @@ -117,7 +117,7 @@ describe('Button', () => { ); const el = queryByText('Loading Button'); - expect(el).toBeDisabled; + expect(el).toBeDisabled(); }); }); From e24a0433260bb085e4bf52dd61321c7cca09eaae Mon Sep 17 00:00:00 2001 From: Wesley Bomar Date: Mon, 13 Jun 2022 19:22:56 -0500 Subject: [PATCH 7/7] Revert "fix(fp-1650): fixes from tup-ui PR" This reverts commit e80f9de1af9160ff8f73bfe906271957b0908d69. --- client/src/components/_common/Button/Button.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/src/components/_common/Button/Button.test.js b/client/src/components/_common/Button/Button.test.js index ad4021af4..eda303e6c 100644 --- a/client/src/components/_common/Button/Button.test.js +++ b/client/src/components/_common/Button/Button.test.js @@ -109,7 +109,7 @@ describe('Button', () => { ); const el = queryByTestId('no button here'); - expect(el).toBeNull(); + expect(el).toBeNull; }); it('disables button when in loading state', () => { muteTypeNotLinkNoSizeLog(); @@ -117,7 +117,7 @@ describe('Button', () => { ); const el = queryByText('Loading Button'); - expect(el).toBeDisabled(); + expect(el).toBeDisabled; }); });