Skip to content
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

chore(NonIdealState): refactored non ideal state component to a functional component #6821

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

chore(NonIdealState): refactored non ideal state component to a functional component #6821

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
79 changes: 37 additions & 42 deletions packages/core/src/components/non-ideal-state/nonIdealState.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import * as React from "react";

import { type IconName, IconSize } from "@blueprintjs/icons";

import { AbstractPureComponent, Classes, DISPLAYNAME_PREFIX, type MaybeElement, type Props } from "../../common";
import { Classes, DISPLAYNAME_PREFIX, type MaybeElement, type Props } from "../../common";
import { ensureElement } from "../../common/utils";
import { H4 } from "../html/html";
import { Icon } from "../icon/icon";
Expand Down Expand Up @@ -79,34 +79,20 @@ export interface NonIdealStateProps extends Props {
*
* @see https://blueprintjs.com/docs/#core/components/non-ideal-state
*/
export class NonIdealState extends AbstractPureComponent<NonIdealStateProps> {
public static displayName = `${DISPLAYNAME_PREFIX}.NonIdealState`;

public static defaultProps: Partial<NonIdealStateProps> = {
iconMuted: true,
iconSize: NonIdealStateIconSize.STANDARD,
layout: "vertical",
};

public render() {
const { action, children, className, layout } = this.props;

export const NonIdealState: React.FC<NonIdealStateProps> = ({
action,
children,
className,
description,
icon,
iconMuted = true,
iconSize = NonIdealStateIconSize.STANDARD,
layout = "vertical",
title,
}) => {
Copy link
Contributor

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 maybeRenderVisual = () => {
return (
<div className={classNames(Classes.NON_IDEAL_STATE, `${Classes.NON_IDEAL_STATE}-${layout}`, className)}>
{this.maybeRenderVisual()}
{this.maybeRenderText()}
{action}
{children}
</div>
);
}

private maybeRenderVisual() {
const { icon, iconMuted, iconSize } = this.props;
if (icon == null) {
return undefined;
} else {
return (
!!icon && (
<div
className={Classes.NON_IDEAL_STATE_VISUAL}
style={{ fontSize: `${iconSize}px`, lineHeight: `${iconSize}px` }}
Expand All @@ -119,21 +105,30 @@ export class NonIdealState extends AbstractPureComponent<NonIdealStateProps> {
tabIndex={-1}
/>
</div>
);
}
}

private maybeRenderText() {
const { description, title } = this.props;
if (title == null && description == null) {
return undefined;
} else {
return (
)
);
};

const maybeRenderText = () => {
Copy link
Contributor

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.

return (
!!title &&
!!description && (
Copy link
Contributor

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

<div className={Classes.NON_IDEAL_STATE_TEXT}>
{title && <H4>{title}</H4>}
{description && ensureElement(description, "div")}
</div>
);
}
}
}
)
);
};

return (
<div className={classNames(Classes.NON_IDEAL_STATE, `${Classes.NON_IDEAL_STATE}-${layout}`, className)}>
{maybeRenderVisual()}
{maybeRenderText()}
{action}
{children}
</div>
);
};

NonIdealState.displayName = `${DISPLAYNAME_PREFIX}.NonIdealState`;