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

feat: allow external icon usage #1644

Merged
merged 1 commit into from
May 3, 2022
Merged

feat: allow external icon usage #1644

merged 1 commit into from
May 3, 2022

Conversation

rabelloo
Copy link
Contributor

@rabelloo rabelloo commented May 2, 2022

Purpose

Some apps have a strict size restriction and can't simply include all <Icons /> provided as an SVG sheet.
That means using Select and Search can have serious size implications.

Currently, their only option is to construct their own SVG sheet, presumably by taking the one provided by @onfido/castor-icons and removing unused icons.

This PR aims to offer an alternative in a per-component API to replace default icons.

Approach

Offer an icon prop that allows overriding default icon.

import { IconSearch } from '@onfido/castor-icons';

<Search icon={<IconSearch />} />

Because the prop accepts a JSX.Element, it is not limited to @onfido/castor-icons, any React component's output will work.

Testing

Modified Select and Search stories, including a new story to demonstrate how to use the new API.

Risks

Elements in component props is not an ergonomic API in my opinion, we should still prefer children and composition.
However this is an exception with limited applications and it prevents weird APIs like wrapping special children in React context.

We could also simply use inline icons in those components and accept the trade-off of more runtime DOM elements but reduced bundle size.
A valid alternative in case we don't want to extend components with the proposed API.

@rabelloo rabelloo added the enhancement New feature or request label May 2, 2022
@rabelloo rabelloo requested a review from josokinas May 2, 2022 14:27
@rabelloo rabelloo self-assigned this May 2, 2022
@vercel
Copy link

vercel bot commented May 2, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
castor ✅ Ready (Inspect) Visit Preview May 2, 2022 at 2:27PM (UTC)

@rabelloo rabelloo merged commit 67c7238 into main May 3, 2022
@rabelloo rabelloo deleted the feat/external-icons branch May 3, 2022 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants