feat(EmptyState): update EmptyState with updates for penta by wise-king-sullyman · Pull Request #9947

feat(EmptyState): update EmptyState with updates for penta by wise-king-sullyman · Pull Request #9947 · patternfly/patternfly-react

Conversation

What: Closes #9928

Additional issues:

Adds the new penta status modifiers via a status prop on the main EmptyState component. That prop will also set our typical status icons automatically for consumer convenience and to try and drive consistency.

Also reworks the component generally a bit. Discussion with @mcoker lead to the realization that design intent requires the title to be present, so having the EmptyStateHeader component passed in composably wasn't ideal. This PR adds EmptyStateHeader and EmptyStateIcon directly into EmptyState and managed by props at the top level for that reason.

Convenience link: https://patternfly-react-pr-9947.surge.sh/components/empty-state

		}

		export type EmptyStateStatus = 'danger' \| 'warning' \| 'success' \| 'info' \| 'custom';
		export type EmptyStateVariantType = 'xs' \| 'sm' \| 'lg' \| 'xl' \| 'full';

Choose a reason for hiding this comment

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

I've gone back and forth on keeping the EmptyStateVariant enum, figured it could be left for a followup PR if we do want to remove it though.

I think we should decide on exporting union types like this or exporting enums more broadly before we make the call for the sake of consistency.

		className?: string;
		/** Content rendered inside the empty state */
		children: React.ReactNode;
		children?: React.ReactNode;

Choose a reason for hiding this comment

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

children changed to optional because titleText has been lifted and changed to required.

Comment on lines +26 to +43

		/** Status style options added to the empty state icon */
		status?: EmptyStateStatus;
		/** Additional class names to apply to the empty state header */
		headerClassName?: string;
		/** Additional props passed to the empty state header container */
		headerProps?: React.HTMLProps<HTMLDivElement>;
		/** Additional classes added to the title inside empty state header */
		titleClassName?: string;
		/** Text of the title inside empty state header, will be wrapped in headingLevel */
		titleText: React.ReactNode;
		/** Empty state icon element to be rendered. Can also be a spinner component */
		icon?: React.ComponentType<any>;
		/** Additional props passed to the icon element */
		iconProps?: EmptyStateIconProps;
		/** The heading level to use, default is h1 */
		headingLevel?: EmptyStateHeadingLevel;

Choose a reason for hiding this comment

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

It seems that I need to explicit include the EmptyStateHeader props here for our props table to work properly. At some point in the future we should be able to remove this and just have EmptyStateProps extend EmptyStateHeaderProps, that will require a docs framework update though.

		);
		}: EmptyStateProps) => {
		const statusIcon = status && statusIcons[status];
		const icon = customIcon \|\| statusIcon;

Choose a reason for hiding this comment

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

One decision point: if we have the status prop set a default icon should the icon prop override it, or the other way around?

I think as a consumer I would prefer the former so that I can have the status apply custom colors to my passed icon, so that's what I went with, but I'm not dead set.

Choose a reason for hiding this comment

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

Agreed. I only have one hesitation with that, but it's something that can be covered in a11y docs (passing an icon that doesn't visually represent the status)


		export interface IconProps extends Omit<React.HTMLProps<SVGElement>, 'size'> {
		/** Changes the color of the icon. */
		color?: string;

Choose a reason for hiding this comment

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

Color prop removed as the status prop will allow color customization in a themed way.

If they really need to consumers can still apply colors directly to the icon they pass in.

		};

		return (
		<EmptyState status={status} titleText={titleMap[status]} headingLevel="h4">

Choose a reason for hiding this comment

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

I think having the status and title change when clicking a button in the example was a nice way of showing the different options, but if we want to just keep this simpler I'm fine to remove these features.

Choose a reason for hiding this comment

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

While I do like it, I might lean more towards using a Select outside the EmptyState itself like we do in other examples. Not a blocker, though, and this could be something we could possibly revisit when we're able to dig into rendering examples with "option toggles" similar to Carbon and other library pages.

If we proceed with this approach we will want code mods to move the EmptyStateHeader props to the EmptyState and update imports accordingly.

Choose a reason for hiding this comment

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

I would suggest grouping all the header related props together into an object.

		children?: React.ReactNode;
		/** Modifies empty state max-width and sizes of icon, title and body */
		variant?: 'xs' \| 'sm' \| 'lg' \| 'xl' \| 'full';
		variant?: EmptyStateVariantType;

Choose a reason for hiding this comment

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

I would actually lean towards making EmptyStateVariantType an enum and list out the union here like we do elsewhere for consistency.

		/** Cause component to consume the available height of its container */
		isFullHeight?: boolean;
		/** Status of the empty state, will set a default status icon and color. Icon can be overwritten using the icon prop */
		status?: EmptyStateStatus;

Choose a reason for hiding this comment

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

Same comment as above here.

		/** Additional class names to apply to the empty state header */
		headerClassName?: string;
		/** Additional props passed to the empty state header container */
		headerProps?: React.HTMLProps<HTMLDivElement>;

Choose a reason for hiding this comment

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

Should this be of type EmptyStateHeaderProps?

Choose a reason for hiding this comment

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

Removed this prop and iconProps per our discussion at the group review.

		<div
		className={css(
		styles.emptyState,
		variant === 'xs' && styles.modifiers.xs,

Choose a reason for hiding this comment

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

can lines 67 -70 be simplified to variant === 'xs' && styles.modifiers.[variant]. if variant id notfull

		import { EmptyStateIconProps } from './EmptyStateIcon';
		import { EmptyStateIcon, EmptyStateIconProps } from './EmptyStateIcon';

		export type EmptyStateHeadingLevel = 'h1' \| 'h2' \| 'h3' \| 'h4' \| 'h5' \| 'h6';