Skip to content

Conversation

@chrisdavies
Copy link
Contributor

Adds support for IP fields to the Lens editor.

@chrisdavies chrisdavies added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.5.0 labels Sep 23, 2019
@chrisdavies chrisdavies requested review from a team, flash1293 and wylieconlon September 23, 2019 19:01
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Code LGTM, didn't test locally

const icons: Partial<Record<string, UnwrapArray<typeof ICON_TYPES>>> = {
boolean: 'invert',
date: 'calendar',
ip: 'compute',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hehe, I had the same problem over in #45384 and chose link. I don't really care about which one it will be, but it should be the same. @cchaos compute or link for IPs?

I will move this component into kibana_react in a separate PR, seems like a good fit for a general purpose component

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely don't think it should be link. We use that more exclusively for actual URL's. compute sort of works, but I worry that it's only recognizable by those with more intimate knowledge of computer parts. I'd lean towards online because it's indicating there's an internet connection. Or if that really doesn't work, maybe storage, otherwise we'd have to create something new.

Screen Shot 2019-09-24 at 08 42 50 AMScreen Shot 2019-09-24 at 08 43 03 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for a specific "IP" icon. I like storage second best till that's ready.

Copy link
Member

Choose a reason for hiding this comment

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

note that with #46485 discover will use the same icons (for now in black, but coloring is already built in), there are a few more here, but I think it would make sense to have 1 shared component just for this icons. A specific IP icon would be welcome, but also the storage one is fine

return Boolean(
newField &&
newField.type === 'string' &&
supportedTypes.has(newField.type) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

oops, thanks for taking care of that.

@flash1293 flash1293 added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Sep 24, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisdavies chrisdavies merged commit fee0699 into elastic:master Sep 24, 2019
@chrisdavies chrisdavies deleted the lens/ip-addresses branch September 24, 2019 20:05
chrisdavies added a commit to chrisdavies/kibana that referenced this pull request Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.5.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants