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 string control #32881

Merged

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Mar 11, 2019

Summary

EUIfication of string control for aggregation parameter.
This control is used for Custom label, Exclude and Include labels on Data tab.

Mar-11-2019 14-40-14

Part of #30922

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

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@maryia-lapata maryia-lapata requested a review from timroes March 11, 2019 15:44
@maryia-lapata maryia-lapata added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Mar 11, 2019
@maryia-lapata maryia-lapata requested a review from cchaos March 11, 2019 15:45
@elasticmachine

This comment has been minimized.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Glanced at code and screenshot, but didn't load it in browser. LGTM. I'm guessing that the auto-suggest in the screenshot is from the browser and not something we implemented, right?

@maryia-lapata
Copy link
Contributor Author

@cchaos yes, it's the auto-suggest from the browser.
Here is the screenshot from the browser in incognito mode:

Mar-12-2019 10-51-34

@timroes timroes mentioned this pull request Mar 12, 2019
29 tasks
@maryia-lapata maryia-lapata added the WIP Work in progress label Mar 13, 2019
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@maryia-lapata
Copy link
Contributor Author

I added some changes according to @timroes's feedback. The screenshot wasn't changed. I also checked accessibility and pseudo locale, it works fine.

@maryia-lapata maryia-lapata removed the WIP Work in progress label Mar 14, 2019
@maryia-lapata maryia-lapata requested review from timroes and dmlemeshko and removed request for timroes March 14, 2019 08:37
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@dmlemeshko dmlemeshko 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 updating the test. LGTM

@timroes timroes requested a review from lukeelmers March 14, 2019 14:31
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested and seems to work, and Code LGTM overall. Since I did some significant parts in this architecture I wonder, if maybe someone else from the team (cc @lukeelmers or @chrisdavies) could give this another review, before we're merging this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

After a guided tour from @timroes, I think that all makes sense now...

Or at least, makes as much sense as the legacy angular world is ever going to make 😉

Thanks for digging into this -- LGTM!

template: function ($el, attrs) {
if (attrs.editorComponent) {
// Why do we need the `ng-if` here?
// Short answer: Preventing black magic
Copy link
Member

Choose a reason for hiding this comment

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

🔮

@timroes timroes added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues v8.0.0 v7.2.0 and removed Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Mar 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata merged commit 9878707 into elastic:master Mar 18, 2019
@maryia-lapata maryia-lapata deleted the vis-editor-agg-param-string-control branch March 18, 2019 08:34
maryia-lapata added a commit to maryia-lapata/kibana that referenced this pull request Mar 18, 2019
* EUIficate string control

* Fix tests

* Update unit test

* Add data-test-subj for input

* Remove unused props

* Refactoring

* Add value prop to be able to discard changes on form

* Refactoring

* Set ng-model

* Remove unnecessary check and specify model name

* Set full width to be consistent with other controls from the form
maryia-lapata added a commit that referenced this pull request Mar 18, 2019
* EUIficate string control

* Fix tests

* Update unit test

* Add data-test-subj for input

* Remove unused props

* Refactoring

* Add value prop to be able to discard changes on form

* Refactoring

* Set ng-model

* Remove unnecessary check and specify model name

* Set full width to be consistent with other controls from the form
@timroes timroes added the release_note:skip Skip the PR/issue when compiling release notes label Mar 27, 2019
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.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants