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

feat(StatusIcon): Declarative approach using union types #49

Merged
merged 6 commits into from
Jan 22, 2021
Merged

feat(StatusIcon): Declarative approach using union types #49

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a28ac9
Declarative approach using union types
gildub Jan 20, 2021
b315a43
Restore retun icon when no label
gildub Jan 20, 2021
83a53d4
Follow standard naming for type
gildub Jan 20, 2021
a447815
Simply use IconListType only
gildub Jan 21, 2021
4e50d92
Fix import and exlamation icons assignation
gildub Jan 21, 2021
89d7d3c
Remove string enums
gildub Jan 21, 2021
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
142 changes: 81 additions & 61 deletions src/components/StatusIcon/StatusIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ import {
global_success_color_100 as successColor,
global_warning_color_100 as warningColor,
global_Color_dark_200 as unknownColor,
global_danger_color_100 as dangerColor,
global_danger_color_100 as errorColor,
global_info_color_100 as infoColor,
global_info_color_200 as loadingColor,
} from '@patternfly/react-tokens';

import './StatusIcon.css';
Expand All @@ -27,8 +28,65 @@ export enum StatusType {
Unknown = 'Unknown',
}

export type StatusIconType = keyof typeof StatusType;

type IconType =
| {
icon: typeof CheckCircleIcon;
color: typeof successColor;
}
| {
icon: typeof ExclamationTriangleIcon;
color: typeof warningColor;
}
| {
icon: typeof ExclamationCircleIcon;
color: typeof errorColor;
}
| {
icon: typeof InfoCircleIcon;
color: typeof infoColor;
}
| {
icon: typeof Spinner;
color: typeof loadingColor;
}
| {
icon: typeof QuestionCircleIcon;
color: typeof unknownColor;
};

type iconListType = { [key in StatusIconType]: IconType };

const iconList: iconListType = {
[StatusType.Ok]: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the use of this iconList as a lookup table, but I'm not sure if it's worth the duplication of this icon-and-color-mapping information in the IconType. We have to sort of repeat the information twice here, and the type is only checking that we repeated it correctly. I think iconListType could just be

type iconListType = { [key in StatusType]: React.Component<SVGIconProps> }

which is what PF uses internally for the icon components (https://github.com/patternfly/patternfly-react/blob/master/packages/react-icons/src/createIcon.tsx#L54).

What I was referring to in the issue was perhaps replacing the StatusType enum itself with:

export type StatusType = 'Ok' | 'Warning' | 'Error' | 'Info' | 'Loading' | 'Unknown';

And then this iconList would just use regular object keys (strings) and TS would enforce that each one is a valid StatusType even without the enum, and that way when using the component you can just pass statusType="Ok" instead of having another import.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least that's one of the takeaways I got from the article. Maybe I'm missing the point 😄

Copy link
Collaborator Author

@gildub gildub Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback.

I agree with the redundant icon-color mapping information.
One advantage of the IconType type is to enforce the defined icon and color mappings but maybe that's not that important here.

As it was discussed in the article's comments, we can also get best of both worlds. So by using an enum for the string values makes it easier to maintain those and exploit them with a union. Meanwhile that's might not be that relevant here.

Copy link
Collaborator

@mturley mturley Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, maybe I'm not fully wrapping my head around why enums are easier to maintain. You're probably right in some contexts but in this specific case I think a simple union of strings provides the same benefits.

As far as enforcing the icon/color mappings, those mappings are built into the component itself so we're not enforcing anything the consumer passes since they don't specify the color. The iconList mapping itself provides that info (the type just enforces that the object matches the type, within the same file.. it seems like it would be just as easy to make a mistake in the type than in the object itself.

I think a useful thing to enforce would be just that the iconList fully covers all the available status types, and all we need for that is:

type iconListType = {
  [key in StatusType]: {
    icon: React.Component<SVGIconProps>;
    color: { name: string; value: string; var: string };
  };
};

maybe I'm trying to over-simplify.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By using enums we can also use them instead of manipulating hard coded strings which was one of the initial incentive to use them:
status === StatusType.Loading vs status === "Loading". We loose that by doing type StatusIconType = "Ok" | "Warning" | "Error" | "Info" | "Loading" | "Unknown";`

Regarding the IconList, you're right the type doesn't enforce anything because the code controls it. So I might have overcomplicated it. Well it's a trial and error approach to build up with the enum/union pattern in this case.
The main issue left is to import for SVGIconProps and SpinnerProps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By using enums we can also use them instead of manipulating hard coded strings

yeah, that was the original reason I liked them, but I realized after trying string unions that using hard-coded strings is just fine if there are only certain allowed ones, IntelliSense and TSC will still autocomplete/check them for you and you don't have to import the enum.

icon: CheckCircleIcon,
color: successColor,
},
[StatusType.Warning]: {
icon: ExclamationCircleIcon,
color: warningColor,
},
[StatusType.Error]: {
icon: ExclamationCircleIcon,
color: errorColor,
},
[StatusType.Info]: {
icon: InfoCircleIcon,
color: infoColor,
},
[StatusType.Loading]: {
icon: Spinner,
color: loadingColor,
},
[StatusType.Unknown]: {
icon: QuestionCircleIcon,
color: unknownColor,
},
};

export interface IStatusIconProps {
status: StatusType;
status: StatusIconType;
label?: React.ReactNode;
isDisabled?: boolean;
className?: string;
Expand All @@ -40,63 +98,25 @@ export const StatusIcon: React.FunctionComponent<IStatusIconProps> = ({
isDisabled = false,
className = '',
}: IStatusIconProps) => {
let icon: React.ReactElement | null = null;
if (status === StatusType.Ok) {
icon = (
<CheckCircleIcon
className={className}
color={isDisabled ? disabledColor.value : successColor.value}
/>
);
}
if (status === StatusType.Warning) {
icon = (
<ExclamationTriangleIcon
className={className}
color={isDisabled ? disabledColor.value : warningColor.value}
/>
);
}
if (status === StatusType.Error) {
icon = (
<ExclamationCircleIcon
className={className}
color={isDisabled ? disabledColor.value : dangerColor.value}
/>
);
}
if (status === StatusType.Info) {
icon = (
<InfoCircleIcon
className={className}
color={isDisabled ? disabledColor.value : infoColor.value}
/>
);
}
if (status === StatusType.Loading) {
icon = <Spinner className={`${className} status-icon-loading-spinner`} />;
}
if (status === StatusType.Unknown) {
icon = (
<QuestionCircleIcon
className={className}
color={isDisabled ? disabledColor.value : unknownColor.value}
/>
);
}
if (label) {
return (
<Flex
spaceItems={{ default: 'spaceItemsSm' }}
alignItems={{ default: 'alignItemsCenter' }}
flexWrap={{ default: 'nowrap' }}
style={{ whiteSpace: 'nowrap' }}
className={className}
>
<FlexItem>{icon}</FlexItem>
<FlexItem>{label}</FlexItem>
</Flex>
);
}
return icon;
const Icon = iconList[status].icon;

return label ? (
<Flex
spaceItems={{ default: 'spaceItemsSm' }}
alignItems={{ default: 'alignItemsCenter' }}
flexWrap={{ default: 'nowrap' }}
style={{ whiteSpace: 'nowrap' }}
className={className}
>
<FlexItem>
<Icon
color={isDisabled ? disabledColor.value : iconList[status].color.value}
className={
status === StatusType.Loading ? `${className} status-icon-loading-spinner` : className
}
/>
</FlexItem>
<FlexItem>{label}</FlexItem>
</Flex>
) : null;
};