-
Notifications
You must be signed in to change notification settings - Fork 357
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.
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
feat(EmptyState): update EmptyState with updates for penta #9947
feat(EmptyState): update EmptyState with updates for penta #9947
Conversation
1a9302e
to
70a3cd5
Compare
@@ -10,37 +11,78 @@ export enum EmptyStateVariant { | |||
full = 'full' | |||
} | |||
|
|||
export type EmptyStateStatus = 'danger' | 'warning' | 'success' | 'info' | 'custom'; | |||
export type EmptyStateVariantType = 'xs' | 'sm' | 'lg' | 'xl' | 'full'; |
There was a problem hiding this comment.
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.
export interface EmptyStateProps extends React.HTMLProps<HTMLDivElement> { | ||
/** Additional classes added to the empty state */ | ||
className?: string; | ||
/** Content rendered inside the empty state */ | ||
children: React.ReactNode; | ||
children?: React.ReactNode; |
There was a problem hiding this comment.
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.
/** 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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Preview: https://patternfly-react-pr-9947.surge.sh A11y report: https://patternfly-react-pr-9947-a11y.surge.sh |
If we proceed with this approach we will want code mods to move the |
There was a problem hiding this comment.
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.
/** Modifies empty state max-width and sizes of icon, title and body */ | ||
variant?: 'xs' | 'sm' | 'lg' | 'xl' | 'full'; | ||
variant?: EmptyStateVariantType; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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>; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment about an enum for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design looks good. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall things look good. Had a question below and replied to a couple of your comments above (nothing blocking).
packages/react-core/src/components/EmptyState/EmptyStateHeader.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L🍕TM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🥃
What: Closes #9928
Additional issues:
Adds the new penta
status
modifiers via astatus
prop on the mainEmptyState
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 addsEmptyStateHeader
andEmptyStateIcon
directly intoEmptyState
and managed by props at the top level for that reason.Convenience link: https://patternfly-react-pr-9947.surge.sh/components/empty-state