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.

Sign up for GitHub

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

Jump to bottom

[Vis: Default editor] Create number_input with numeric value required #48117

Merged
merged 7 commits into from
Oct 31, 2019
Merged

[Vis: Default editor] Create number_input with numeric value required #48117

merged 7 commits into from
Oct 31, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Oct 14, 2019 •
edited
Loading

Summary

This resolves #49611 .

Created a reuired_number_input_option for future usage inside Options tab in the default editor.
This will validate empty values and store the to the state params as null.
The more description about the feature request could be find in #49611

image

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof mentioned this pull request Oct 16, 2019
Merged
7 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof marked this pull request as ready for review October 23, 2019 09:44
@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@sulemanof sulemanof added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Oct 23, 2019

Jenkins, test this - flaky discover test

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes requested a review from flash1293 October 25, 2019 10:39
flash1293
flash1293 reviewed Oct 28, 2019
View reviewed changes
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.

This seems to work fine (threshold line values validated correctly). I'm guessing but is the code for number input duplicated because setValidity isn't passed in everywhere which would cause errors? What if I need a non-required number input, then this comment doesn't really help, right? https://github.com/elastic/kibana/pull/48117/files#diff-b2e640da85879261119c39ecbe0479c9R39

A little PR description would be helpful

@sulemanof sulemanof mentioned this pull request Oct 29, 2019
Closed
3 tasks
@sulemanof
Copy link
Contributor Author

@flash1293 thanks for pushing me up 😊

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

flash1293
flash1293 approved these changes Oct 29, 2019
View reviewed changes
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.

LGTM, thanks for the documentation 👍

@sulemanof sulemanof merged commit 9e00c78 into elastic:master Oct 31, 2019
@sulemanof sulemanof deleted the fix/number_input_types branch October 31, 2019 08:28
@sulemanof sulemanof mentioned this pull request Oct 31, 2019
Merged
sulemanof added a commit that referenced this pull request Oct 31, 2019
@sulemanof
[Vis: Default editor] Create number_input with numeric value required (…
5d9020a 
…#48117) (#49840)

* Created number_input with numeric value required

* Remove extra license

* Make value be possibly null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flash1293 flash1293 flash1293 approved these changes

@maryia-lapata maryia-lapata Awaiting requested review from maryia-lapata

@timroes timroes Awaiting requested review from timroes

Assignees
No one assigned
Labels
backported release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Vis: Default editor] Use required numeric input fields in options tab
4 participants
@sulemanof @elasticmachine @timroes @flash1293