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(tag): introducing custom icon #7262

Merged

Conversation

IgnacioBecerra
Copy link
Contributor

@IgnacioBecerra IgnacioBecerra commented Nov 10, 2020

Related: carbon-design-system/carbon-for-ibm-dotcom#3669

This is a contribution for the Tag component in which allows the adopter to include any icon from @carbon/icons before the content text. This feature had been requested as part of the Expressive theme in Carbon for IBM.com.

The new renderIcon prop requires an icon component as an argument, which will render it accordingly within the Tag component. hasCustomIcon must also be passed into the Tag component to activate the custom icon functionality.

Here are some examples:

Using renderIcon: Tag16
Screen Shot 2020-11-11 at 10 00 00 AM

Using renderIcon: Compass16
Screen Shot 2020-11-11 at 10 00 40 AM

Changelog

New

  • added renderIcon prop to allow for a custom icon to be added before the content text

Testing / Reviewing

Enable/disable the new prop, experiment with various icon names to ensure they render properly.

@IgnacioBecerra IgnacioBecerra requested a review from a team as a code owner November 10, 2020 19:46
@emyarod emyarod requested review from a team and shinytoyrobots and removed request for a team November 10, 2020 19:47
@ghost ghost requested a review from johnbister November 10, 2020 19:47
@emyarod emyarod requested review from a team and removed request for shinytoyrobots, johnbister and a team November 10, 2020 19:47
@ghost ghost requested a review from johnbister November 10, 2020 19:47
@netlify
Copy link

netlify bot commented Nov 10, 2020

✔️ Deploy preview for carbon-elements ready!

🔨 Explore the source changes: 77a582d

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/60078ef9ad126d00078bf949

😎 Browse the preview: https://deploy-preview-7262--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Nov 10, 2020

✔️ Deploy preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 77a582d

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-components-react/deploys/60078ef94a18630008ac552a

😎 Browse the preview: https://deploy-preview-7262--carbon-components-react.netlify.app

@jeanservaas
Copy link
Collaborator

Hey @IgnacioBecerra thanks for the contribution!

I was just looking at Shixie's spec I think I would tweak the spacing a bit

She's actually modified our tag for digital (.com team) and is using:

  • 14/20pt type
  • 32px tag height (6px of padding top and bottom)
  • Gray 80 (your type is too light for contrast accessibility right now)
    - I changed her spec to make the space between the icon and the type 4px (instead of 8px)

image

If you wanted a spec to add an icon to the Carbon tag, for product teams (which it looks like you're doing), here it is:

  • 12/16pt type
  • 24px tag height (6px of padding top and bottom)
  • Gray 80 (your type is too light for contrast accessibility right now)
  • Make the space between the icon and the type 4px (instead of 8px)

image

@mjabbink
Copy link

mjabbink commented Nov 10, 2020

Icon cropping in the left corner

96F8F21B-C620-4A13-85DD-EC5A8017C8AB

@IgnacioBecerra
Copy link
Contributor Author

@jeanservaas Thank you for the specs! I modified the text color values to type 80 in tokens.scss to conform with contrast accessibility guidelines.

@tw15egan Thank you for the examples! Using the renderIcon prop, an icon component will be passed as a prop into the Tag component instead of using strings. The Storybook knob for custom icons will be gone however, although it's not strictly necessary as the other components lack it as well.

@mjabbink Thanks for letting me know about that! I wasn't able to replicate the cropping, but I removed the css property that I suspect was the cause of the issue.

@tw15egan
Copy link
Collaborator

@IgnacioBecerra Awesome! If you do want to allow a user to see that they can change the icon to whatever they want, you can add in a knob, like how we do for the button story. Here's the link if you're interested

@IgnacioBecerra
Copy link
Contributor Author

@tw15egan Thanks! Added three examples in the Storybook using the select knob.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking the time to work on this! 👍 ✅ 🚀

packages/components/src/components/tag/_tag.scss Outdated Show resolved Hide resolved
packages/react/src/components/Tag/Tag-story.js Outdated Show resolved Hide resolved
packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
@IgnacioBecerra
Copy link
Contributor Author

@emyarod Thanks. for the suggestions! I refactored using if/else if for better readability, let me know if you have any thoughts about it!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

it looks like you are not using an autoformatter in your editor, can you try running yarn format in the repository root?

packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@andreancardona andreancardona merged commit 31e19ca into carbon-design-system:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants