Skip to content

feat(Spinner): remove legacy variant and svg prop #8616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 8, 2023
Merged

feat(Spinner): remove legacy variant and svg prop #8616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d51357
feat(Spinner): remove legacy variant and svg prop
gitdallas Jan 27, 2023
fb6ef29
nullish coalescing
gitdallas Jan 27, 2023
4458e88
remove unecessary object for attribute
gitdallas Jan 27, 2023
41aaea0
proper type for spinner svg props
gitdallas Jan 27, 2023
7a2e58c
use raw svg for component, cast prop as cssproperties
gitdallas Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions packages/react-core/src/components/Icon/Icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,7 @@ export const Icon: React.FunctionComponent<IconComponentProps> = ({
defaultProgressArialabel = 'Loading...',
...props
}: IconComponentProps) => {
const _progressIcon = progressIcon ? (
progressIcon
) : (
<Spinner diameter="1em" isSVG aria-label={defaultProgressArialabel} />
);
const _progressIcon = progressIcon ?? <Spinner diameter="1em" aria-label={defaultProgressArialabel} />;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a prop for this now! 🥳 It's beta, but the icon component is beta, too. IMO we could promote isInline any time, it's really simple, so it should be promoted before or with <Icon>

Suggested change
const _progressIcon = progressIcon ?? <Spinner diameter="1em" aria-label={defaultProgressArialabel} />;
const _progressIcon = progressIcon ?? <Spinner isInline aria-label={defaultProgressArialabel} />;

If we make this update, can you also update this comment? We could replace "1em" with "inline", or just remove that and say "Defaults to a spinner."

patternfly-react/packages/react-core/src/components/Icon/Icon.tsx

Line 9 in 7a2e58c

/** Icon when isInProgress is set to true. Defaults to a 1em spinner. */

And if it's not scope creep, could we poke around and see if this is used anywhere else? I looked in the react-core dir and only found this one. I can open a separate issue if you'd prefer.

patternfly-react/packages/react-core/src/components/Dropdown/DropdownToggleCheckbox.tsx

Line 93 in 25b7097

diameter="1em"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcoker - looks like this was merged soon after your comments... lmk if you open a new issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolethoen @tlabaj do you see any reason to hold off on using this beta prop (<Spinner isInline />)? Or maybe just go ahead and promote the prop?


return (
<span
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Icon/examples/IconCustomProgress.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export const IconProgress: React.FunctionComponent = () => {
name="toggle-icon-progress-custom"
/>
</div>
<Icon isInProgress={isInProgress} progressIcon={<Spinner diameter="2em" isSVG aria-label="Loading..." />}>
<Icon isInProgress={isInProgress} progressIcon={<Spinner diameter="2em" aria-label="Loading..." />}>
<CheckCircleIcon />
</Icon>
</React.Fragment>
Expand Down
47 changes: 16 additions & 31 deletions packages/react-core/src/components/Spinner/Spinner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@ export enum spinnerSize {
xl = 'xl'
}

export interface SpinnerProps extends Omit<React.HTMLProps<HTMLSpanElement>, 'size'> {
export interface SpinnerProps extends React.SVGProps<SVGSVGElement> {
/** Additional classes added to the Spinner. */
className?: string;
/** Size variant of progress. */
size?: 'sm' | 'md' | 'lg' | 'xl';
/** Text describing that current loading status or progress */
'aria-valuetext'?: string;
/** Whether to use an SVG (new) rather than a span (old) */
isSVG?: boolean;
/** Diameter of spinner set as CSS variable */
diameter?: string;
/** @beta Indicates the spinner is inline and the size should inherit the text font size. This will override the size prop. */
Expand All @@ -33,37 +31,24 @@ export const Spinner: React.FunctionComponent<SpinnerProps> = ({
className = '',
size = 'xl',
'aria-valuetext': ariaValueText = 'Loading...',
isSVG = true,
diameter,
isInline = false,
'aria-label': ariaLabel,
'aria-labelledBy': ariaLabelledBy,
...props
}: SpinnerProps) => {
const Component = isSVG ? 'svg' : ('span' as any);

return (
<Component
className={css(styles.spinner, isInline ? styles.modifiers.inline : styles.modifiers[size], className)}
role="progressbar"
aria-valuetext={ariaValueText}
{...(isSVG && { viewBox: '0 0 100 100' })}
{...(diameter && { style: { '--pf-c-spinner--diameter': diameter } })}
{...(ariaLabel && { 'aria-label': ariaLabel })}
{...(ariaLabelledBy && { 'aria-labelledBy': ariaLabelledBy })}
{...(!ariaLabel && !ariaLabelledBy && { 'aria-label': 'Contents' })}
{...props}
>
{isSVG ? (
<circle className={styles.spinnerPath} cx="50" cy="50" r="45" fill="none" />
) : (
<React.Fragment>
<span className={css(styles.spinnerClipper)} />
<span className={css(styles.spinnerLeadBall)} />
<span className={css(styles.spinnerTailBall)} />
</React.Fragment>
)}
</Component>
);
};
}: SpinnerProps) => (
<svg
className={css(styles.spinner, isInline ? styles.modifiers.inline : styles.modifiers[size], className)}
role="progressbar"
aria-valuetext={ariaValueText}
viewBox="0 0 100 100"
{...(diameter && { style: { ['--pf-c-spinner--diameter']: diameter } as React.CSSProperties })}
{...(ariaLabel && { 'aria-label': ariaLabel })}
{...(ariaLabelledBy && { 'aria-labelledBy': ariaLabelledBy })}
{...(!ariaLabel && !ariaLabelledBy && { 'aria-label': 'Contents' })}
{...props}
>
<circle className={styles.spinnerPath} cx="50" cy="50" r="45" fill="none" />
</svg>
);
Spinner.displayName = 'Spinner';
8 changes: 1 addition & 7 deletions packages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,4 @@ test('large spinner', () => {
test('extra large spinner', () => {
const { asFragment } = render(<Spinner size="xl" />);
expect(asFragment()).toMatchSnapshot();
});

test('non-SVG spinner', () => {
const { asFragment } = render(<Spinner isSVG={false} />)

expect(asFragment()).toMatchSnapshot();
})
});
21 changes: 0 additions & 21 deletions packages/react-core/src/components/Spinner/__tests__/__snapshots__/Spinner.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,27 +60,6 @@ exports[`medium spinner 1`] = `
</DocumentFragment>
`;

exports[`non-SVG spinner 1`] = `
<DocumentFragment>
<span
aria-label="Contents"
aria-valuetext="Loading..."
class="pf-c-spinner pf-m-xl"
role="progressbar"
>
<span
class="pf-c-spinner__clipper"
/>
<span
class="pf-c-spinner__lead-ball"
/>
<span
class="pf-c-spinner__tail-ball"
/>
</span>
</DocumentFragment>
`;

exports[`simple spinner 1`] = `
<DocumentFragment>
<svg
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Spinner/examples/SpinnerBasic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import { Spinner } from '@patternfly/react-core';

export const SpinnerBasic: React.FunctionComponent = () => <Spinner isSVG aria-label="Contents of the basic example" />;
export const SpinnerBasic: React.FunctionComponent = () => <Spinner aria-label="Contents of the basic example" />;
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Spinner/examples/SpinnerCustomSize.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ import React from 'react';
import { Spinner } from '@patternfly/react-core';

export const SpinnerCustomSize: React.FunctionComponent = () => (
<Spinner isSVG diameter="80px" aria-label="Contents of the custom size example" />
<Spinner diameter="80px" aria-label="Contents of the custom size example" />
);
10 changes: 5 additions & 5 deletions packages/react-core/src/components/Spinner/examples/SpinnerInline.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,23 @@ export const SpinnerInline: React.FunctionComponent = () => (
<TextContent>
<Text component="h1">
Heading
<Spinner isInline isSVG aria-label="Spinner in a heading" />
<Spinner isInline aria-label="Spinner in a heading" />
</Text>
<Text component="p">
Lorem ipsum dolor sit amet, consectetur adipiscing elit Sed hendrerit nisi in cursus maximus.
</Text>
<Text component="h2">
Second level
<Spinner isInline isSVG aria-label="spinner in a subheading" />
<Spinner isInline aria-label="spinner in a subheading" />
</Text>
<Text component="p">
Curabitur accumsan turpis pharetra blandit. Quisque condimentum maximus mi,{' '}
<Spinner isInline isSVG aria-label="Spinner in a paragraph" /> sit amet commodo arcu rutrum id. Proin pretium
urna vel cursus venenatis. Suspendisse potenti.
<Spinner isInline aria-label="Spinner in a paragraph" /> sit amet commodo arcu rutrum id. Proin pretium urna vel
cursus venenatis. Suspendisse potenti.
</Text>
<small>
Sometimes you need small text
<Spinner isInline isSVG aria-label="Spinner in a small element" />
<Spinner isInline aria-label="Spinner in a small element" />
</small>
</TextContent>
</React.Fragment>
Expand Down
8 changes: 4 additions & 4 deletions packages/react-core/src/components/Spinner/examples/SpinnerSizeVariations.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import { Spinner } from '@patternfly/react-core';

export const SpinnerSizeVariations: React.FunctionComponent = () => (
<React.Fragment>
<Spinner isSVG size="sm" aria-label="Contents of the small example" />
<Spinner isSVG size="md" aria-label="Contents of the medium example" />
<Spinner isSVG size="lg" aria-label="Contents of the large example" />
<Spinner isSVG size="xl" aria-label="Contents of the extra large example" />
<Spinner size="sm" aria-label="Contents of the small example" />
<Spinner size="md" aria-label="Contents of the medium example" />
<Spinner size="lg" aria-label="Contents of the large example" />
<Spinner size="xl" aria-label="Contents of the extra large example" />
</React.Fragment>
);