-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
chore(NonIdealState): refactored non ideal state component to a functional component #6821
base: develop
Are you sure you want to change the base?
chore(NonIdealState): refactored non ideal state component to a functional component #6821
Conversation
Generate changelog in
|
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.
This is almost ready; requesting a few minor stylistic and one functional change.
iconSize = NonIdealStateIconSize.STANDARD, | ||
layout = "vertical", | ||
title, | ||
}) => { |
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 we destructure these inside of the function body rather than in the arguments? I think this is more in line with the structure we use for other functional components that have multiline props in Blueprint.
export const NonIdealState: React.FC<NonIdealStateProps> = props => {
const {
action,
children,
className,
description,
icon,
iconMuted = true,
iconSize = NonIdealStateIconSize.STANDARD,
layout = "vertical",
title,
} = props;
// ...
}
); | ||
}; | ||
|
||
const maybeRenderText = () => { |
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.
Instead of keeping these as inline functions, can we encapsulate the conditional in a couple variables, say:
const shouldRenderVisual = icon != null;
const shouldRenderText = title != null || description != null;
and then inline them below:
<div className={classNames(Classes.NON_IDEAL_STATE, `${Classes.NON_IDEAL_STATE}-${layout}`, className)}>
{shouldRenderVisual && (
<div
className={Classes.NON_IDEAL_STATE_VISUAL}
style={{ fontSize: `${iconSize}px`, lineHeight: `${iconSize}px` }}
>
<Icon
className={classNames({ [Classes.ICON_MUTED]: iconMuted })}
icon={icon}
size={iconSize}
aria-hidden={true}
tabIndex={-1}
/>
</div>
)}
{shouldRenderText && (
<div className={Classes.NON_IDEAL_STATE_TEXT}>
{title && <H4>{title}</H4>}
{description && ensureElement(description, "div")}
</div>
)}
{action}
{children}
</div>
I think this would improve the code clarity a bit, and it precludes having to make additional unnecessary function calls.
const maybeRenderText = () => { | ||
return ( | ||
!!title && | ||
!!description && ( |
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 believe the logic here should be:
!!title || !!description
Otherwise, the text will only render only if both title and description are set. Having just a title and no description (or vice versa) is a valid configuration for this component.
Better yet, can we be a bit more explicit with our operators here? (since this is TypeScript)
title != null || description != null
Fixes #6289
Checklist
Changes proposed in this pull request:
Reviewers should focus on:
Screenshot