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.

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

[Enhance] Implement Validation for Icon Input Source & Set Default Icon to Beaker #1137

Merged
merged 7 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- Update ouiTextSubduedColor in `next` dark theme ([#973](https://github.com/opensearch-project/oui/pull/973))
- Add `crossInCircleEmpty` and `power` icons ([#1044](https://github.com/opensearch-project/oui/pull/1044))
- Add `Figma` icon and link to OUI Figma resources ([#1064](https://github.com/opensearch-project/oui/pull/1064))
- Implement validation for icon input source & set default icon to `Beaker` ([#1137](https://github.com/opensearch-project/oui/pull/1137))

### 🐛 Bug Fixes

Expand Down
52 changes: 51 additions & 1 deletion src/components/icon/__snapshots__/icon.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -10184,7 +10184,7 @@ exports[`OuiIcon props type wrench is rendered 1`] = `
</svg>
`;

exports[`OuiIcon renders custom components 1`] = `
exports[`OuiIcon render different types of icons renders custom components 1`] = `
<span
aria-hidden="true"
aria-label="heart"
Expand All @@ -10195,3 +10195,53 @@ exports[`OuiIcon renders custom components 1`] = `
❤️
</span>
`;

exports[`OuiIcon render different types of icons renders custom svg from absolute url 1`] = `
<img
alt=""
class="ouiIcon ouiIcon--medium"
src="https://upload.wikimedia.org/wikipedia/commons/0/02/SVG_logo.svg"
/>
`;

exports[`OuiIcon render different types of icons renders custom svg from relative url 1`] = `
<img
alt=""
class="ouiIcon ouiIcon--medium"
src="./assets/beaker.svg"
/>
`;

exports[`OuiIcon render different types of icons renders default icon when type is not in OuiIconType 1`] = `
<svg
aria-hidden="true"
class="ouiIcon ouiIcon--medium ouiIcon-isLoading"
focusable="false"
height="16"
role="img"
viewBox="0 0 16 16"
width="16"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M5.277 10.088c.02.014.04.03.057.047.582.55 1.134.812 1.666.812.586 0 1.84-.293 3.713-.88L9 6.212V2H7v4.212l-1.723 3.876Zm-.438.987L3.539 14h8.922l-1.32-2.969C9.096 11.677 7.733 12 7 12c-.74 0-1.463-.315-2.161-.925ZM6 2H5V1h6v1h-1v4l3.375 7.594A1 1 0 0 1 12.461 15H3.54a1 1 0 0 1-.914-1.406L6 6V2Z"
/>
</svg>
`;

exports[`OuiIcon render different types of icons renders icon when type is in cache 1`] = `
<svg
aria-hidden="true"
class="ouiIcon ouiIcon--medium"
focusable="false"
height="16"
role="img"
viewBox="0 0 16 16"
width="16"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M0 1.994C0 .893.895 0 1.994 0h12.012C15.107 0 16 .895 16 1.994v12.012A1.995 1.995 0 0 1 14.006 16H1.994A1.995 1.995 0 0 1 0 14.006V1.994Zm1 0v12.012c0 .548.446.994.994.994h12.012a.995.995 0 0 0 .994-.994V1.994A.995.995 0 0 0 14.006 1H1.994A.995.995 0 0 0 1 1.994ZM1 4h14v1H1V4Zm1.5-1a.5.5 0 0 1 0-1h1a.5.5 0 0 1 0 1h-1Zm3 0a.5.5 0 0 1 0-1h1a.5.5 0 0 1 0 1h-1Zm4.947 6.106a1 1 0 0 1 0 1.788l-3 2A1 1 0 0 1 6 12V8a1 1 0 0 1 1.447-.894l3 2ZM10 10 7 8v4l3-2Z"
/>
</svg>
`;
55 changes: 41 additions & 14 deletions src/components/icon/icon.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -135,23 +135,11 @@ describe('OuiIcon', () => {
});
});

it('renders custom components', () => {
const CustomIcon = ({ ...props }) => {
return (
<span role="img" aria-label="heart" {...props}>
❤️
</span>
);
};
const component = mount(<OuiIcon type={CustomIcon} />);
expect(prettyHtml(component.html())).toMatchSnapshot();
});

describe('appendIconComponentCache', () => {
it('does nothing if not called', () => {
const component = mount(<OuiIcon type="videoPlayer" />);
expect(
component.find('OuiIcon[type="videoPlayer"] > OuiIconEmpty').length
component.find('OuiIcon[type="videoPlayer"] > OuiIconBeaker').length
).toBe(1);
});

Expand All @@ -172,8 +160,47 @@ describe('OuiIcon', () => {
});
const component = mount(<OuiIcon type="accessibility" />);
expect(
component.find('OuiIcon[type="accessibility"] > OuiIconEmpty').length
component.find('OuiIcon[type="accessibility"] > OuiIconBeaker').length
).toBe(1);
});
});

describe('render different types of icons', () => {
it('renders icon when type is in cache', () => {
appendIconComponentCache({
videoPlayer: OuiIconVideoPlayer,
});
const component = mount(<OuiIcon type="videoPlayer" />);
expect(prettyHtml(component.html())).toMatchSnapshot();
});

it('renders custom svg from absolute url', () => {
const component = mount(
<OuiIcon type="https://upload.wikimedia.org/wikipedia/commons/0/02/SVG_logo.svg" />
);
expect(prettyHtml(component.html())).toMatchSnapshot();
});

it('renders custom svg from relative url', () => {
const component = mount(<OuiIcon type="./assets/beaker.svg" />);
expect(prettyHtml(component.html())).toMatchSnapshot();
});
Comment on lines +184 to +187
Copy link
Member

Choose a reason for hiding this comment

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

nit - maybe it's better to test custom svg rendering with an asset that isn't used as a fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuarrrr should I raise an issue so that we can revise this unit test or just leave it as it is?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, if you think it's a worthwhile change.


it('renders default icon when type is not in OuiIconType', () => {
const component = mount(<OuiIcon type="foo" />);
expect(prettyHtml(component.html())).toMatchSnapshot();
});

it('renders custom components', () => {
const CustomIcon = ({ ...props }) => {
return (
<span role="img" aria-label="heart" {...props}>
❤️
</span>
);
};
const component = mount(<OuiIcon type={CustomIcon} />);
expect(prettyHtml(component.html())).toMatchSnapshot();
});
});
});
52 changes: 29 additions & 23 deletions src/components/icon/icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import { CommonProps, keysOf } from '../common';
// because we'd need to dynamically know if we're importing the
// TS file (dev/docs) or the JS file (distributed), and it's more effort than worth
// to generate & git track a TS module definition for each icon component
import { icon as empty } from './assets/empty.js';
import { icon as defaultIcon } from './assets/beaker.js';
import { enqueueStateChange } from '../../services/react';

import { htmlIdGenerator } from '../../services';
Expand Down Expand Up @@ -582,22 +582,16 @@ interface State {
neededLoading: boolean; // controls the fade-in animation, cached icons are immediately rendered
}

function isOuiIconType(x: OuiIconProps['type']): x is OuiIconType {
return typeof x === 'string' && typeToPathMap.hasOwnProperty(x);
function isOuiIconType(type: OuiIconProps['type']): type is OuiIconType {
return typeof type === 'string' && typeToPathMap.hasOwnProperty(type);
}

function getInitialIcon(icon: OuiIconProps['type']) {
if (icon == null) {
return undefined;
}
if (isOuiIconType(icon)) {
if (iconComponentCache.hasOwnProperty(icon)) {
return iconComponentCache[icon];
}
return undefined;
}
function isUrl(type: OuiIconProps['type']) {
return typeof type === 'string' && (type.includes('.') || type.includes('/'));
}

return icon;
function isCachedIcon(type: OuiIconProps['type']) {
return isOuiIconType(type) && iconComponentCache.hasOwnProperty(type);
}

const generateId = htmlIdGenerator();
Expand Down Expand Up @@ -628,13 +622,22 @@ export class OuiIcon extends PureComponent<OuiIconProps, State> {
super(props);

const { type } = props;
const initialIcon = getInitialIcon(type);
let initialIcon;
let isLoading = false;

if (isOuiIconType(type) && initialIcon == null) {
// Category 1: cached oui icons
if (isCachedIcon(type)) {
initialIcon = iconComponentCache[type as string];
// Category 2: URL (relative, absolute)
} else if (isUrl(type)) {
initialIcon = type;
// Category 3: non-cached oui icon or new icon
} else if (typeof type === 'string') {
isLoading = true;
this.loadIconComponent(type);
this.loadIconComponent(type as OuiIconType);
} else {
// Category 4: custom icon component
initialIcon = type;
this.onIconLoad();
}
Comment on lines +628 to 642
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be rewritten to not use a mutable variable. I'll make a followup issue, no need to worry about pushing it in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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


Expand Down Expand Up @@ -683,12 +686,14 @@ export class OuiIcon extends PureComponent<OuiIconProps, State> {
return;
}

const iconPath = typeToPathMap[iconType] || 'beaker';

import(
/* webpackChunkName: "icon.[request]" */
// It's important that we don't use a template string here, it
// stops webpack from building a dynamic require context.
// eslint-disable-next-line prefer-template
'./assets/' + typeToPathMap[iconType] + '.js'
'./assets/' + iconPath + '.js'
).then(({ icon }) => {
iconComponentCache[iconType] = icon;
enqueueStateChange(() => {
Expand Down Expand Up @@ -761,7 +766,7 @@ export class OuiIcon extends PureComponent<OuiIconProps, State> {
className
);

const icon = this.state.icon || (empty as ComponentType);
const icon = this.state.icon || (defaultIcon as ComponentType);

// This is a fix for IE and Edge, which ignores tabindex="-1" on an SVG, but respects
// focusable="false".
Expand All @@ -771,6 +776,7 @@ export class OuiIcon extends PureComponent<OuiIconProps, State> {
// - For all other values, the consumer wants the icon to be focusable.
const focusable = tabIndex == null || tabIndex === -1 ? 'false' : 'true';

// relative, absolute path
if (typeof icon === 'string') {
return (
<img
Expand All @@ -784,15 +790,15 @@ export class OuiIcon extends PureComponent<OuiIconProps, State> {
} else {
const Svg = icon;

// If it's an empty icon, or if there is no aria-label, aria-labelledby, or title it gets aria-hidden true
// If it's a default icon, or if there is no aria-label, aria-labelledby, or title it gets aria-hidden true
const isAriaHidden =
icon === empty ||
icon === defaultIcon ||
!(
this.props['aria-label'] ||
this.props['aria-labelledby'] ||
this.props.title
);
const hideIconEmpty = isAriaHidden && { 'aria-hidden': true };
const hideDefaultIcon = isAriaHidden && { 'aria-hidden': true };

let titleId: any;

Expand All @@ -817,7 +823,7 @@ export class OuiIcon extends PureComponent<OuiIconProps, State> {
title={title}
{...titleId}
{...rest}
{...hideIconEmpty}
{...hideDefaultIcon}
/>
);
}
Expand Down
Loading