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

fix(Icon): Support null tagName in Icon component #6981

Merged
merged 2 commits into from
Sep 19, 2024
Merged

fix(Icon): Support null tagName in Icon component #6981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6ce75c
Ensure createElement in Icon is never null
ggdouglas Sep 17, 2024
2a3cffc
Update test to check for presence of span element
ggdouglas Sep 17, 2024
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
2 changes: 1 addition & 1 deletion packages/core/src/components/icon/icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export const Icon: IconComponent = React.forwardRef(function <T extends Element>
: size === IconSize.LARGE
? Classes.ICON_LARGE
: undefined;
return React.createElement(tagName!, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to ensure this value is always a valid element. The alternative would be to do something like a null check and return early, e.g.

if (tagName == null) {
    return null;
}
return React.createElement(tagName, { ... });

The former will show a blip of the fallback while the icon paths load:

Screenshot 2024-09-17 at 16 21 42

whereas the later will appear and shift in without any placeholder at all:

Screenshot 2024-09-17 at 16 22 34

Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to use || here over ???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I suppose the only main difference is that || will use "span" as the default for any falsy value, including "", 0, and false. If the user is consuming this component in TypeScript, there is functionally no difference since tagName is either type ofReact.JSX.IntrinsicElements or null.

Do we care about this edge case for non-TypeScript consumers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah okay, that makes sense. I think its fair to keep this for non-TS consumers.

return React.createElement(tagName || "span", {
"aria-hidden": title ? undefined : true,
...removeNonHTMLProps(htmlProps),
className: classNames(
Expand Down
3 changes: 2 additions & 1 deletion packages/core/test/icon/iconTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ describe("<Icon>", () => {

it("allows specifying the root element as <svg> when tagName={null}", () => {
const handleClick: React.MouseEventHandler<SVGSVGElement> = () => undefined;
mount(<Icon<SVGSVGElement> icon="add" onClick={handleClick} tagName={null} />);
const wrapper = mount(<Icon<SVGSVGElement> icon="add" onClick={handleClick} tagName={null} />);
assert.isFalse(wrapper.find("span").exists());
});

/** Asserts that rendered icon has an SVG path. */
Expand Down