-
Notifications
You must be signed in to change notification settings - Fork 842
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
Replace app icons with new, lighter set #1225
Conversation
@JessSmithSup New icons look spiffy 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one seems to be missing the accent color:
Reversing colors
As I look more at it, the main one that I'm concerned with is the visualizeApp
. I'd really like to see the bars as the dark color because that's the main objective of the app, not the axes. I see the point about the dark color being on the outside, but I'd rather make sure the icon is clear and readable.
I'd love to see the icon be a little more dynamic than just three bars too, to try to express that it's more than just a bar chart. I keep coming back to this one that I created a while back to try to represent "x/y" charts, but it won't be used for that and so it might be nice to use it for the whole app.
src-docs/src/views/icon/apps.js
Outdated
'notebookApp', | ||
'packetbeatApp', | ||
'securityAnalyticsApp', | ||
'sqlApp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you alphabetize this list so it's easier to scan in the docs page?
Could put the the beats ones together though at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still love to these alphabetized. VSCode has a really easy way to do it: command + shift + p -> sort lines
@@ -0,0 +1,11 @@ | |||
<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"> | |||
<g> | |||
<rect width="2" height="7" x="7" y="17" fill="#00BFB2"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we worried about this color not having a high contrast (2.308) with a white background? We could bump it to #00A69B
which would at least get us to 3.031 (AA 18+)
Teal color change - If we use these on the marketing site we aren’t going to change the color as we already have icons in this style and they could possibly live together. I prefer no shift, or if we are worried about it I’d rather just use a one color icon Reversing colors - I’m totally ok with reversing the colors. Visualize icon - If we feel strongly we can go back, my opinion is keeping these icons simple is best. We don’t have to make each icon a checklist of every functionality of the feature. The fact that this icon is distinct and also simple is good with me, adding more elements will make the level of detail inconsistent. |
Teal color change - Yeah I'm not sure if it matters much, but thought I'd at least make people aware. Visualize icon - I agree that keeping the icons simple is a good thing but I don't think my suggestion was any more complicated or noisy than most of the other icons (like index management). I do feel kind of strongly mainly because the vis app is such a main app and the icon is just kind of meh. (And no, it's not just because I had thought up the icon 6 months ago, I promise ;). I really do think it's a better representative of the entire vis app. |
@cchaos lmk if you're down with this |
I'm digging it. Would it be simpler if the dots weren't hollow? |
@cchaos that's how this set is treating circles so I'd like to keep that part consistent |
@JessSmithSup Thoughts then on changing the hollow bars to lines like the ones representing lines on paper and the canvas app? |
@cchaos I think I'm into it... |
OK. Latest update made the following changes.
Ignore timelion icon being old. It's a Kibana thing AfterBeforeColor overwrite documentation |
@JessSmithSup Too many lines? |
jenkins, test this |
@cchaos yeah, I think maybe keeping it at 3 is nice. @snide breaking my heart with that color change lol. Are you saying the teal is too hot next to the blue labels? Shifting this goes away from the consistency of branding. Any reason those labels aren't bold black like the ones on top? I guess because they are links? Not sure I'm anymore into that dark teal next to the dark blue. I think a lighter teal is a nicer contrast. I would lobby against a dark shift. |
Lemme spin up some tests. I'll get you some screens so you can see it a couple different ways.
Yep. That's the reason. |
My personal opinion is that the EUI primary blue and teal in the icons clash - they simply don't work well together. Short term, I favor the mono icon; longer term, a page redesign should be considered as there is a lot going on here with the number, size, and proximity of icons to one another. |
As Ryan said, the main issue is that the new teal clashes with our primary blue which is different from the marketing primary blue which is much lighter. We can't lighten our primary blue to match because our buttons and text must be AA contrast level compliant. That makes the teal clash really with our whole color scheme because of we've got such darker muted colors for accessibility. I don't think it's a big deal to darken the teal for use in Kibana as we don't (and can't) match the marketing site anyway. We might be able to get away with something in between our secondary color and this teal. I mentioned that The other option is just to swap the teal for primary blue? |
I think I'm maybe not feeling any of the teal/blue mixes, compliancy aside. Also are icons required to be compliant? Marketing site side we are going to keep these the original teal/cool gray because that is the "elastic brand teal" and we have icons that look great in this style and fit with our brand. While I would love to uphold brand in the UI, I get there are more requirements and limitations. So, I think monochromatic would be cleaner. That dark teal/primary EUI blue is straying too far. Controversial thought for this page.... what if they were monochromatic EUI primary blue? Either that or keep it all monochromatic dark cool gray. Basically, anything to not have that dark teal/dark blue combo. LMK thoughts |
Also we're shifting our brand main blue |
@JessSmithSup ^^ good point and worth exploring. |
@snide Can you try a sample where the default is the original teal, but in the bottom half of the homepage where they're inside a link they're monochromatic blue? |
a76328b
to
b02d1af
Compare
b02d1af
to
1d81c77
Compare
Talked this over with @ryankeairns and @cchaos this morning. We came up with the following:
Now, with that said I also want to say we're SUPER OPEN to Jess telling us what EUI's overall palette should be, what is our blue, what is our teal...etc. As long as it passes our accessibility needs then we're happy to hand that stuff over. I think that's a discussion we can do at EAH, and as you've seen, is trivial for us to change. Let's figure it all out in person. In the meantime, I'm going to merge this PR because overall it is a MEGA-IMPROVEMENT over what we have now, and it gives us the flexibility to make the Kibana side whatever we want it to be. I agree with @ryankeairns that the Kibana home page needs a rethink, and that (more than the color of the icons) is our biggest problem. @cchaos can you do a final code / visual scan when you have a moment. |
Sure thing but did you mean: "two-toned teal, mono text and mono blue" not a two-toned blue version? |
@cchaos correct. MonoSecondary (which will be the default)Primary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple doc suggestions. But, LGTM
src-docs/src/views/icon/apps.js
Outdated
'notebookApp', | ||
'packetbeatApp', | ||
'securityAnalyticsApp', | ||
'sqlApp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still love to these alphabetized. VSCode has a really easy way to do it: command + shift + p -> sort lines
@@ -43,5 +43,20 @@ export default () => ( | |||
</EuiFlexItem> | |||
)) | |||
} | |||
<EuiFlexItem | |||
className="guideDemo__icon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHANGELOG.md
Outdated
@@ -1,12 +1,13 @@ | |||
## [`master`](https://github.com/elastic/eui/tree/master) | |||
|
|||
- Added TypeScript typings for `EuiKeyPadMenu` ([#1229](https://github.com/elastic/eui/pull/1229)) | |||
- Force `EuiPopover` contents to stick to its initial position when the content changes ([#1199](https://github.com/elastic/eui/pull/1199)) | |||
- Forced `EuiPopover` contents to stick to its initial position when the content changes ([#1199](https://github.com/elastic/eui/pull/1199)) | |||
- Updated `EuiIcon` app icon set and allow them to adjust two-toned colorschemes ([#1225](https://github.com/elastic/eui/pull/1225)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer adjusting the "two-toned" color [ ] schemes.
For those playing at home, here's how things finally landed with the coloring of the icons. @cchaos this also addresses your feedback on docs. |
Apologies for the latest response ever. It was more difficult than I thought to get stable internet in an airport. This all looks/sounds good. Thanks for putting so much thought into it! |
cc to @gjones, @ryankeairns, @formgeist, @cchaos, @JessSmithSup, @Michalwo, @Adrian-J that this is incoming. I believe we'll have a solutions / product set following up in a bit as well.
Summary
Switcheroo of EUI's app icon set based on @JessSmithSup's work. Process was the following.
.euiIcon
itself, which could have its own animations applied) to fix some blurriness on Firefox. Hat tip to @cchaos for saving me a couple hours on that one 🎩Issues
Questions
Checklist
Any props added have proper autodocsThis was checked against keyboard-only and screenreader scenarios