Icon & CustomIcon component

jaredcwhite commented 1 year ago

Issue bloom-housing/bloom#3248

Some notes on the PR:

There was discussion around the icon naming since we decided to split Icon into two components, one for FontAwesome and one for bespoke SVGs. I'd proposed using Icon for FontAwesome and CustomIcon for any SVG and implemented that in this PR, but feedback is welcome.

I also tried out using Storybook Controls and separate .tsx files for stories in this PR. To access controls switch to the "Canvas" tab when viewing in Storybook. There's just one so far, the size prop.

Also a note regarding code style…so I've been following a lot of good thinking out there around when to (or not to) use classes in CSS and the class attribute vs. other sorts of attributes like data- and aria- and so forth for variations/states on components. Some recommended reading:

The waster potential of attribute selectors in CSS Style with Stateful, Semantic Selectors Represent State with HTML Attributes, Not Class Names CSS Classes considered harmful

In light of this, I thought I'd experiment here on this first component that has size variations. So instead of a class name like icon-size-utility-sm or is-size-utility-sm or whatever, I went with [data-size="utility-sm"]. So in the final markup, instead of <span class="icon icon-size-utility-sm">, it's <span class="icon" data-size="utility-sm">. If we wanted to really go nuts, we could even wade into the waters of custom element names and attributes, for something like <bloom-icon size="utility-sm">, but given we're not building web components but React components, that's probably a bridge too far. I like the idea of using data- (or when applicable, aria-) for variations, but I welcome your feedback and discussion.

netlify[bot] commented 1 year ago

Deploy Preview for storybook-ui-seeds ready!

Name	Link
Latest commit	9a35f8a382a4ecfb2c6416dd80d4df6887198055
Latest deploy log	https://app.netlify.com/sites/storybook-ui-seeds/deploys/6407e91a9fb3700008251503
Deploy Preview	https://deploy-preview-9--storybook-ui-seeds.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

slowbot commented 1 year ago

@jaredcwhite

I know we landed on separate components but I am waffling on whether or not that makes sense in this context. I like how Vibe just makes custom an option of a single component https://style.monday.com/?path=/docs/media-icon--overview
Nit, but can we also use icons from the subset we have identified here https://www.figma.com/file/SsYt5YsgbmOPbwr5Qk2RKB/bloom-core-library?node-id=2395%3A6764&t=aDJTnEajyOvLV0EQ-1
Not sure if I have a hard opinion on class or no class but I think we would need to document that so other devs would know when you use class and when not to

jaredcwhite commented 1 year ago

@slowbot Sure, I can update the examples to use icons from your set. And I'm also starting to waffle now if we do need them to be separate components. Might want a team confab to take a second look.

Regarding the class question, I would say this is an "internal" implementation detail so whether the component's using class or using data-*, it doesn't matter much from a component consumer standpoint because they'd use the same React prop either way.

jaredcwhite commented 1 year ago

@ludtkemorgan @slowbot All right, I've merged the two components into a single component. Essentially it seemed the easiest solution was to use an icon prop for the FontAwesome definitions, and just regular children for SVG nodes. Let me know what you think.

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 1.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

bloom-housing / ui-seeds