Skip to content

[EuiIcon] Added support for ghost and text colors on logos#5245

Merged
cchaos merged 5 commits intoelastic:masterfrom
cchaos:fix/ghost_logos
Oct 6, 2021
Merged

[EuiIcon] Added support for ghost and text colors on logos#5245
cchaos merged 5 commits intoelastic:masterfrom
cchaos:fix/ghost_logos

Conversation

@cchaos
Copy link
Contributor

@cchaos cchaos commented Oct 5, 2021

While working in Kibana and wanting to put our Cloud logo within a "dark" colored Collapsible nav group, I realized our logos don't support the icon color "ghost". Creative Services does allow the solid versions of our logos to be used, but only in black (text color) or white.

Since these SVG's have their fills declared directly on the paths, I had to target several nesting layers and use the !important to override their fill to use currentColor provided by the color prop of EuiIcon.

I've updated the example text and added a button group to see the different available colors.

Gif too big: https://share.getcloudapp.com/GGupOGJQ
Screen Shot 2021-10-05 at 19 15 27 PM

Note that the Elastic logo does not support these colors. (it would just fill to a blob) So I added a data-type="logoElastic" to the svg itself (which does get compiled into the TSX) for which I set a :not() selector for. I think this may even be something we could improve on in the icon compiler to automatically add the iconType string to the exported .tsx file as the data-type attribute. This would help a ton in consuming applications when we're trying to find specific instances via the dev console.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs and playground toggles
  • Added documentation
  • [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos cchaos requested review from cee-chen and elizabetdev October 5, 2021 23:13
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5245/

@snide
Copy link
Contributor

snide commented Oct 6, 2021

Seems like we could set up the SVG for the Elastic one differently and then write some custom CSS for the stroke (making it trasparent) if we wanted?

Only reason I bring this up is we do show the logo in pure iconic styling on Tshirts and stuff. Doesn't seem to go against brand. 💯 don't want to hold up this PR, just qualifying the reasoning, so we can follow up later if we want.

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

LGTM! Tested in Chrome, Safari, Edge, and Firefox. 🎉

For the elastic logo, we could use the monochromatic versions for ghost and text: https://brand.elastic.co/302f66895/p/06c73c-elastic-logo/b/1243b6.

I just have one CSS suggestion.

@cchaos
Copy link
Contributor Author

cchaos commented Oct 6, 2021

@miukimiu & @snide I took both of your advice, made the selector more specific to those just with a fill using *[fill] and then adjusted the Elastic Logo version to have the white outline actually be an outline (originally it was a solid shape behind all the nodes) and created some styles specifically for that.
Screen Recording 2021-10-06 at 12 15 55 PM

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5245/

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

This looks amazing to me, but you already knew that!! 😉

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@cchaos cchaos enabled auto-merge (squash) October 6, 2021 17:41
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5245/

@cchaos cchaos merged commit e5ef323 into elastic:master Oct 6, 2021
@cchaos cchaos deleted the fix/ghost_logos branch October 6, 2021 18:54
ym pushed a commit to ym/eui that referenced this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants