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 icons to Add Data list and tutorial view #16592

Merged
merged 2 commits into from
Feb 13, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Feb 8, 2018

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexfrancoeur
Copy link

❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ @nreese!!

@formgeist @snide what's the expected look and feel for tutorials without icons? Also, from a design perspective I imagine the card views can get pretty "colorful". Do we want a monochromatic feel here at all so that all cards are equal? For example, my eyes automatically jump to Redis because it's red.

@@ -13,7 +13,6 @@ export function netflowSpecProvider() {
' indexes the events into Elasticsearch, and installs a suite of Kibana dashboards.' +
' This module support Netflow Version 5 and 9.' +
' [Learn more]({config.docs.logstash}/netflow-module.html).',
//iconPath: '', TODO
Copy link
Contributor

@ycombinator ycombinator Feb 8, 2018

Choose a reason for hiding this comment

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

Is Netflow not going to get an icon? Ever (EDIT: otherwise we should leave the TODO here)? I noticed that it wasn't in the screenshot either. @alexfrancoeur?

Choose a reason for hiding this comment

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

@ycombinator I'm open to any icon here. I've pinged about screenshots a few times now too, would love to see one in there. Think you could add one? We could use a Cisco icon for Netflow, I can't find any others. We have a few things like system metrics / logs that do not have an icon either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, IIRC I used the Cisco icon in the modules UI prototype. If we can, I think that's the closest one we can use here. But I'm not sure about the legality of using it (I'd asked about this in an earlier issue: https://github.com/elastic/dev/issues/837#issuecomment-339625666).

So I'd say Cisco if legal is good with it or the generic/default icon if not.

Copy link
Contributor

@ycombinator ycombinator Feb 8, 2018

Choose a reason for hiding this comment

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

As for screenshots, I don't know how to create Netflow data w/o an actual Cisco device or something. I'm sure there is a way to simulate it though. @jsvd any ideas?

Choose a reason for hiding this comment

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

@ycombinator I've ran icons by legal previously, I think we're good here. Let's add the Cisco icon. I'll link to it in the issue

@formgeist
Copy link
Contributor

@alexfrancoeur @snide I think we should have a default icon for those that currently don't have one. Some sort of Add data icon;

screenshot 2018-02-08 23 40 58

On the colourful logos, I'm not sure - obviously the Redis is pretty stark in contrast to many of the others, but I also feel the pages will suffer if the icons are all grey or a single default blue. And then they're really hard to tell apart - I probably wouldn't instantly recognize the Redis logo if it was blue or grey.

@snide
Copy link
Contributor

snide commented Feb 8, 2018

Yeah, I'd keep the color just because I generally think it's wrong to touch other people's branding. I think it's fine that they pop.

I'd leave it as it, rather than make an icon for the generic options, because it would be confused that product's logo. For example, we wouldn't want netflow to have a generic logo there, we should proper add it.

@alexfrancoeur
Copy link

alexfrancoeur commented Feb 12, 2018

Sounds good. If all looks good from everyone else, I'm good with merging as-is and open a new issue for Netflow screenshots / icons.

@ycombinator
Copy link
Contributor

Sounds good. If all looks good from everyone else, I'm good with merging as-is and open a new issue for Netflow screenshots / icons.

It would be nice to get the Netflow icon as part of this PR as it has other icons as well. That said, if we're trying to make some deadline here then I'm good with a follow up PR for the Netflow icon.

++ on separate PR for Netflow screenshots.

@alexfrancoeur
Copy link

I think it may take some time to get the Netflow icon if we plan to use the Cisco logo. Seems that there is a bit of a process on the Cisco side (https://www.cisco.com/c/en/us/about/brand-center/logo-usage-guidelines.html). I don't mind going through this process and checking with legal again, just don't want to hold up this PR waiting for Netflow. Does that work Shaunak?

@ycombinator
Copy link
Contributor

Sure. My main motivation for holding up this PR was to use it as a forcing function for us to not forget about the Netflow module and let its icon slip through the cracks. I've created #16684 so we can stay on top of it. Lets move any further conversation (legal and such) about the Netflow/Cisco icon there.

@nreese nreese force-pushed the home_tutorial_icons branch from b7ced86 to 0e50e5c Compare February 12, 2018 20:28
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

LGTM - technically, I'll defer to the others on whether we should merge this without the netflow icon.

@nreese
Copy link
Contributor Author

nreese commented Feb 13, 2018

@alexfrancoeur @ycombinator Are you ok with this PR getting merged as-is, without the netflow icon?

@alexfrancoeur
Copy link

@nreese yes, I'm good to go. I think we need to do a bit of cleanup with Netflow anyway (like including screenshot). @ycombinator created #16684 to track. It seems like it's a complicated process to use the Cisco icon. That may block this effort and I don't want that to block this PR.

@nreese nreese merged commit 62f3b80 into elastic:master Feb 13, 2018
nreese added a commit to nreese/kibana that referenced this pull request Feb 13, 2018
* add icons for home tutorials

* add test for Synopsis component
@ycombinator
Copy link
Contributor

Yep, I'm good too.

nreese added a commit that referenced this pull request Feb 13, 2018
* add icons for home tutorials

* add test for Synopsis component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add icons to Add Data list and tutorial view
7 participants