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

Add viewBox attribute to SVG & JS logo icons where missing #2240

Merged
merged 4 commits into from
Aug 21, 2019

Conversation

jfsiii
Copy link

@jfsiii jfsiii commented Aug 20, 2019

Summary

closes #2227

Added viewBox values (copy-pasted from logo_rabbitmq.{js,svg}) to Docker, Kubernetes, and Redis logos.

Ran yarn compile-icons and checked in Storybook with Light & Dark Mode.

Pasted React components directly in my plugin

Current master

Screen Shot 2019-08-20 at 10 43 06 AM

This PR

Screen Shot 2019-08-20 at 10 41 42 AM

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation 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

@jfsiii jfsiii requested a review from a team August 20, 2019 14:50
@cchaos cchaos requested review from cchaos and removed request for a team August 20, 2019 15:32
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 still needs a changelog entry. Probably under the bug fix heading. And don't forget to make it past-tense. Thanks!

@jfsiii
Copy link
Author

jfsiii commented Aug 20, 2019

@cchaos I added the changelog entry in f6b7d82

I'm not familiar with the EUI norms so just let me know if you want me to merge the PR or do anything else.

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.

Looks like you'll also need to rebase from master as a release was just cut a couple hours ago.

@jfsiii
Copy link
Author

jfsiii commented Aug 20, 2019

ok. just did a rebase & force push

CHANGELOG.md Outdated
@@ -15,6 +15,7 @@ No public interface changes since `13.4.0`.
- Added missing `onChange` TS defs for EuiRange ([#2211](https://github.com/elastic/eui/pull/2211))
- Fixed `EuiBadge` text cursor to default pointer ([#2234](https://github.com/elastic/eui/pull/2234))
- Fixed `EuiPageContent` className prop to allow the passed-in className to take cascade precedence over classes generated by the component ([#2237](https://github.com/elastic/eui/pull/2237))
- Added `viewBox` attribute to Docker, Kubernetes, and Redis logos ([#2240](https://github.com/elastic/eui/pull/2240))
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the rebase and release, you'll now need to move this line to be under master. And if you don't mind, also copy the **Bug fixes** header up there as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this is confusing, there just happened to be a release while your PR is up.

Copy link
Author

Choose a reason for hiding this comment

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

np. updated in 99c8f55

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.

Perfect, thanks @jfsiii ! You are good to merge!

@jfsiii jfsiii merged commit 718e59b into elastic:master Aug 21, 2019
thompsongl pushed a commit to thompsongl/eui that referenced this pull request Sep 10, 2019
…#2240)

* Add `viewBox` attribute to SVG & JS logo icons where missing

* Update unit test snapshots

* Update changelog

* Move changelog entry to `master`
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.

Some logos are missing a viewBox
2 participants