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

Removes <use> and <def> from svg icons #2162

Closed
wants to merge 8 commits into from

Conversation

mdefazio
Copy link
Contributor

@mdefazio mdefazio commented Jul 24, 2019

Summary

Closes #1989
Removes <use> and <def> from svg icons.

Checklist

- [ ] This was checked in mobile-
- [ ] This was checked in IE11-
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately
    - [ ] This was checked for breaking changes and labeled appropriately
    - [ ] Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@mdefazio mdefazio requested review from snide, cchaos and ryankeairns July 24, 2019 20:20
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Just saw a couple lingering id attributes. But all the icons still look the same rendered.

You'll need to rebase with master as well.

CHANGELOG.md Outdated
@@ -1,6 +1,7 @@
## [`master`](https://github.com/elastic/eui/tree/master)

- Added `partial` glyph to `EuiIcon` ([#2152](https://github.com/elastic/eui/pull/2152))
- Removes <use> and <def> from svg icons ([#2162](https://github.com/elastic/eui/pull/2162))
Copy link
Contributor

Choose a reason for hiding this comment

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

We like to write our changelog entries in the past tense, so you can just change this to 'Removed'

<g>
<use xlink:href="#boxes_vertical-a"/>
</g>
<path id="boxes_vertical-a" d="M7,1 L7,3 L9,3 L9,1 L7,1 Z M6,0 L10,0 L10,4 L6,4 L6,0 Z M6,6 L10,6 L10,10 L6,10 L6,6 Z M7,7 L7,9 L9,9 L9,7 L7,7 Z M6,12 L10,12 L10,16 L6,16 L6,12 Z M7,13 L7,15 L9,15 L9,13 L7,13 Z"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

lingering id on this one

<g>
<use xlink:href="#brush-a"/>
</g>
<path id="brush-a" d="M11.9928058,8.16982791 C11.9928058,9.00087929 11.3203437,9.67717623 10.4940048,9.67717623 L5.4980016,9.67717623 C4.67166267,9.67717623 3.99920064,9.00087929 3.99920064,8.16982791 L3.99920064,6.66247959 L11.9928058,6.66247959 L11.9928058,8.16982791 Z M8.99520384,14.1680693 C8.99520384,14.6232885 8.54656275,14.9951011 7.9960032,14.9951011 C7.44544365,14.9951011 6.99680256,14.6232885 6.99680256,14.1680693 L6.99680256,10.6820751 L8.99520384,10.6820751 L8.99520384,14.1680693 Z M3.99920064,5.65758071 L3.99920064,1.00489888 L5.26119105,1.00489888 L5.26119105,5.65758071 L3.99920064,5.65758071 Z M6.26039169,5.65758071 L6.26039169,1.00489888 L7.50439648,1.00489888 L7.50439648,5.65758071 L6.26039169,5.65758071 Z M8.50359712,5.65758071 L8.50359712,1.00489888 L9.73860911,1.00489888 L9.73860911,5.65758071 L8.50359712,5.65758071 Z M10.7378098,5.65758071 L10.7378098,1.00489888 L11.9918066,1.00489888 L11.9918066,5.65758071 L10.7378098,5.65758071 Z M3.00799361,-1.8e-15 L3,8.16982791 C3,9.55457857 4.12110312,10.6820751 5.4980016,10.6820751 L5.99760192,10.6820751 L5.99760192,14.1680693 C5.99760192,15.1779927 6.89388489,16 7.9960032,16 C9.0981215,16 9.99440448,15.1779927 9.99440448,14.1680693 L9.99440448,10.6820751 L10.4940048,10.6820751 C11.8709033,10.6820751 12.9920064,9.55457857 12.9920064,8.16982791 L13,-1.8e-15 L3.00799361,-1.8e-15 Z"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

id here

<g>
<use xlink:href="#pencil-a"/>
</g>
<path id="pencil-a" d="M12.1480732,3.14807323 L11,2 L2,11 L2,14 L5,14 L14,5 L12.8560732,3.85607323 L4.854,11.854 C4.756,11.951 4.628,12 4.5,12 C4.372,12 4.244,11.951 4.146,11.854 C3.951,11.658 3.951,11.342 4.146,11.146 L12.1480732,3.14807323 Z M11,1 C11.256,1 11.512,1.098 11.707,1.293 L14.707,4.293 C15.098,4.683 15.098,5.317 14.707,5.707 L5.707,14.707 C5.52,14.895 5.265,15 5,15 L2,15 C1.448,15 1,14.552 1,14 L1,11 C1,10.735 1.105,10.48 1.293,10.293 L10.293,1.293 C10.488,1.098 10.744,1 11,1 Z M5,14 L2,14 L2,11 L5,14 Z" />
Copy link
Contributor

Choose a reason for hiding this comment

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

id here

* Added `fullWidth` and `isInvalid` props to EuiFilePicker

* Added a ‘large’ vs 'default' version

* Setting the height statically so that it doesn't shift it's surrounding contents around

* Remove z-indexes: Clashed with any popovers and wasn’t necessary since dom order signifies this

* Added isLoading state (overrides clear button)
Copy link
Contributor

@ryankeairns ryankeairns 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 cleaning these up!
I see that a number of these still contain either fillRule or fill-rule attributes which we typically remove from new SVGs. That said, I'm not 100% certain they can all be removed without causing potentially altering the shape. It's something we could consider cleaning up in a future PR.

@snide
Copy link
Contributor

snide commented Jul 25, 2019

That last merge brought in some old PRs (filepicker stuff). I'll help @mdefazio clean up later today.

@mdefazio mdefazio mentioned this pull request Jul 25, 2019
1 task
@mdefazio
Copy link
Contributor Author

Replaced with #2169

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.

Clean up icon <defs> and <use> usages
4 participants