feat(EmptyState): new component #1514
Conversation
YossiSaadi
force-pushed
the
feat/yossi/EmptySate---new-component-5075443870
branch
from
7eaaa0d
to
a67ccbc
Compare
| export interface EmptyStateProps extends VibeComponentProps { | ||
| imgSrc: string; | ||
| title: string; | ||
| body: string; |
There was a problem hiding this comment.
Not sure if body is the best name for it... Maybe just text, message or smth else?
| imgSrc: string; | ||
| title: string; | ||
| body: string; | ||
| onPrimaryActionClick?: () => void; | ||
| primaryActionLabel?: string; | ||
| onSecondaryActionClick?: () => void; | ||
| secondaryActionLabel?: string; | ||
| imgClassName?: string; |
There was a problem hiding this comment.
Some description comments would be nice
| <img src={imgSrc} alt={title} className={cx(styles.image, imgClassName)} /> | ||
| <Flex direction={Flex.directions.COLUMN} gap={Flex.gaps.SMALL} align={Flex.align.CENTER}> | ||
| <Heading type={Heading.types.H2}>{title}</Heading> | ||
| <Text type={Text.types.TEXT1}>{body}</Text> |
There was a problem hiding this comment.
Always one line here by design?
There was a problem hiding this comment.
I have a concern about what's gonna happen if people put a lot of text here - attaching screenshots to demonstrate
We should think about some max-width limitations, so the text will just go ellipsis with a tooltip when it's reaching the limits
| {secondaryActionLabel && ( | ||
| <Button kind={Button.kinds.TERTIARY} onClick={onSecondaryActionClick}> | ||
| {secondaryActionLabel} | ||
| </Button> | ||
| )} | ||
| {primaryActionLabel && <Button onClick={onPrimaryActionClick}>{primaryActionLabel}</Button>} |
There was a problem hiding this comment.
Nice, but people might use secondaryButton without the primary one, which won't be good. Perhaps, we can protect from this from the beginning
|
Maybe also worth adding RelatedComponents, if u can think of any - just to be consistent |
|
I'd also consider what's needed to be done here (same story in LegacyHeading also) Maybe we can just add some text and reference the new component there |
great suggestion, for the meantime Yael and I agreed that since we need to "run" with it quick, the story would be very minimal |
| export interface EmptyStateProps extends VibeComponentProps { | ||
| imgSrc: string; | ||
| title: string; | ||
| body: string; |
| import Button from "../Button/Button"; | ||
|
|
||
| export interface EmptyStateProps extends VibeComponentProps { | ||
| imgSrc: string; |
There was a problem hiding this comment.
an empty state would also need support SVG, Lottie and maybe at some point different assets. why not keep this as a general node? this would also make imgClassName redundant
| <Heading type={Heading.types.H2}>{title}</Heading> | ||
| <Text type={Text.types.TEXT1}>{body}</Text> |
There was a problem hiding this comment.
general suggestion - apply Heading and Text only when string type is passed, so in cases where other content will be passed, it will not always be wrapped with the typography component constaraints
| <Heading type={Heading.types.H2}>{title}</Heading> | ||
| <Text type={Text.types.TEXT1}>{body}</Text> |
There was a problem hiding this comment.
this would apply ellipsis by default if I understand correctly, not sure this is the expected behavior for the empty state
| onPrimaryActionClick?: () => void; | ||
| primaryActionLabel?: string; |
There was a problem hiding this comment.
I like the abstraction to have very simple actionable button props, but take into consideration that if in the future some a11y props will be needed, we might want to pass the entire props easily primaryActionProps
|
@shlomitc @SergeyRoyt you had great suggestions, I'll go over the content/design-related also with Yael/Meirav |
https://monday.monday.com/boards/3532714909/pulses/5075443870