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

[Vis: Default editor] EUIficate IP Ranges #36896

Merged
merged 23 commits into from
Jun 4, 2019

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented May 22, 2019

Summary

EUIfication of IP Ranges control for aggregation parameter in Default Editor, Data tab.
Part of #30922.

Steps to reproduce: create Data Table visualization, choose IPv4 Range aggregation in Buckets group.

Details

  • If there is no default value defined in the config, it will be set the following default values: { from: '0.0.0.0', to: '255.255.255.255'} and { mask: '0.0.0.0/1' }
  • If a user doesn't set a value, it will be * by default.

This PR also contains removing unused directives: validateIp and validateCidrMask.

Screenshots:

  1. From/To

image

--
2) CIDR mask

image


  1. From/To with default value

May-27-2019 17-51-53

  1. From/To validation

May-27-2019 17-55-06

  1. CIDR mask with default value

May-27-2019 17-56-37


Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata requested a review from sulemanof May 27, 2019 07:58
@maryia-lapata maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues v7.3.0 v8.0.0 labels May 27, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@timroes timroes mentioned this pull request May 27, 2019
29 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata marked this pull request as ready for review May 27, 2019 15:05
@maryia-lapata maryia-lapata requested a review from cchaos May 27, 2019 15:05
@maryia-lapata maryia-lapata added review release_note:skip Skip the PR/issue when compiling release notes labels May 28, 2019
@kertal kertal self-requested a review May 28, 2019 06:38
@maryia-lapata maryia-lapata requested a review from alexwizp May 28, 2019 07:58
@maryia-lapata
Copy link
Contributor Author

@kertal I created a PR with fix for not clickable button with svg: elastic/eui#1985

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Functionality LGTM, but I'd like more type safety in the spot I mentioned. We should be avoiding any types.


export type InputModel =
| InputModelBase & { [model: string]: InputItem }
| InputModelBase & InputItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

This type is currently meaningless for the reason above- the union of [key: string]: any and anything else allows all values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I unified Mask and FromTo models and updated InputModel definition. Please take a look.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

There seems to be a problem at CIDR mask validation, 0.0.0.0/123d validates correctly

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata
Copy link
Contributor Author

There seems to be a problem at CIDR mask validation, 0.0.0.0/123d validates correctly

@kertal thank you for noticing, it was a bug in initial version of CidrMask class. I fixed it.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal kertal self-requested a review June 3, 2019 14:28
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes, Code LGTM, tested in Chrome / Firefox. We should test it again in Safari/IE11 when your EUI fix is released

@maryia-lapata maryia-lapata merged commit b5c7520 into elastic:master Jun 4, 2019
@maryia-lapata maryia-lapata deleted the vis-editor-ip-ranges branch June 4, 2019 09:04
maryia-lapata added a commit to maryia-lapata/kibana that referenced this pull request Jun 4, 2019
* Create IpRangeType and IpRanges controls

* Add validation

* Refactoring

* Add behavior when discarding changes

* Refactoring: create common input list

* Remove old template

* Move add btn to input_list, add placeholder

* Remove unused directives

* Remove unused translations

* Refactoring

* Use EuiButtonGroup instead of toggle button

* Update options ids, add aria-labels

* Remove unused translations

* Update mask model, update TS, update aria labels

* Add validation for CIRD mask
maryia-lapata added a commit that referenced this pull request Jun 4, 2019
* Create IpRangeType and IpRanges controls

* Add validation

* Refactoring

* Add behavior when discarding changes

* Refactoring: create common input list

* Remove old template

* Move add btn to input_list, add placeholder

* Remove unused directives

* Remove unused translations

* Refactoring

* Use EuiButtonGroup instead of toggle button

* Update options ids, add aria-labels

* Remove unused translations

* Update mask model, update TS, update aria labels

* Add validation for CIRD mask
jgowdyelastic pushed a commit that referenced this pull request Jun 4, 2019
* Create IpRangeType and IpRanges controls

* Add validation

* Refactoring

* Add behavior when discarding changes

* Refactoring: create common input list

* Remove old template

* Move add btn to input_list, add placeholder

* Remove unused directives

* Remove unused translations

* Refactoring

* Use EuiButtonGroup instead of toggle button

* Update options ids, add aria-labels

* Remove unused translations

* Update mask model, update TS, update aria labels

* Add validation for CIRD mask
Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

Great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants