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

Added casesApp feature icon #5779

Merged
merged 2 commits into from
Apr 12, 2022
Merged

Added casesApp feature icon #5779

merged 2 commits into from
Apr 12, 2022

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 12, 2022

As provided by @JessSmithSup; cc @cnasikas

Screen Shot 2022-04-11 at 23 59 31 PM

Screen Shot 2022-04-11 at 23 59 36 PM

Also, I edited the SVGO plugin set to exclude removeUselessStrokeAndFill because sometimes fillRule is absolutely necessary. Otherwise the icon was rendering without the hole in the magnifier:

Screen Shot 2022-04-11 at 23 58 42 PM

Checklist

  • Checked 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 and cypress tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

cchaos added 2 commits April 12, 2022 00:04
- Edited the SVGO plugin set to exclude `removeUselessStrokeAndFill` because sometimes `fillRule` is absolutely necessary
@cchaos cchaos requested a review from cee-chen April 12, 2022 04:11
@kibanamachine
Copy link

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

Copy link
Member

@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.

Not sure if it matters, but the removeUselessStrokeAndFill change worried me slightly with the # of snapshot diffs. Should we modify the originating SVGs to remove their fillRule attributes so that their end snapshots don't change? (since they weren't present previously, presumably we don't need them).

I'm probably overthinking how much fill-rule even does though, so TBH I'm fine either way :)

@cchaos
Copy link
Contributor Author

cchaos commented Apr 12, 2022

Yeah, sorry I meant to mention, that I double checked those icons and they're fine with and without that setting. There was technically only 11 icons that changed for that setting, so I didn't think it a big deal to have to go through all the SVG's.

The one thing that got me though, is that without that setting some icons did still keep that fill-rule in their .tsx version. I just don't know when it decides to keep it or remove it, and no matter the amount of changing the .svg would keep that setting during the compiling.

It's not a harmful setting, just sometimes unnecessary.

@cee-chen
Copy link
Member

Thanks for that QA!! Agreed that's super weird and the rule seems a bit buggy/inconsistent/not super well documented. I'm good with turning it off!

@cchaos cchaos merged commit aa7e893 into elastic:main Apr 12, 2022
@cchaos cchaos deleted the icon/cases branch April 12, 2022 15:46
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.

3 participants