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] EUIficate top_sort param editor #36629

Merged
merged 12 commits into from
May 29, 2019
Merged

[Vis: Default editor] EUIficate top_sort param editor #36629

merged 12 commits into from
May 29, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented May 16, 2019 •
edited
Loading

Summary

Part of #30922.
EUIfication of top_sort control for Top Hit aggregation parameter, which contains OrderParamEditor and FieldParamEditor.

top_sort

Also deleted unused directives auto_select_if_only_one.js and scroll_bottom.js.

Common behavior for fields: if there is only one available field it will be selected by default (replaces auto_select_if_only_one directive)

Checklist

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

For maintainers

@sulemanof sulemanof requested a review from maryia-lapata May 16, 2019 14:32
@sulemanof sulemanof added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes v7.2.0 v8.0.0 labels May 16, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

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

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

maryia-lapata
maryia-lapata suggested changes May 20, 2019
View reviewed changes
Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

Please address the comments below

src/legacy/ui/public/agg_types/controls/field.tsx Show resolved Hide resolved
test/functional/apps/visualize/_tile_map.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

maryia-lapata
maryia-lapata approved these changes May 22, 2019
View reviewed changes
Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

Tested locally on Chrome, LGTM

dmlemeshko
dmlemeshko approved these changes May 22, 2019
View reviewed changes
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.

I only reviewed changes related to the functional tests:
Ran several viz suites on both Chrome and Firefox (MacOS 10.14.4) without issues. LGTM.

@sulemanof
Copy link
Contributor Author

@cchaos @wylieconlon could you please take a look? 😊

@sulemanof sulemanof added v7.3.0 and removed v7.2.0 labels May 28, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cchaos
cchaos approved these changes May 28, 2019
View reviewed changes
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.

UI replacement LGTM, just a quick capitalization issue

src/legacy/ui/public/agg_types/controls/top_sort_field.tsx Outdated Show resolved Hide resolved
wylieconlon
wylieconlon approved these changes May 28, 2019
View reviewed changes
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.

LGTM, tested locally

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata merged commit 38b95b1 into elastic:master May 29, 2019
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request May 29, 2019
@sulemanof @maryia-lapata
[Vis: Default editor] EUIficate top_sort param editor (elastic#36629)
e402154
@maryia-lapata maryia-lapata mentioned this pull request May 29, 2019
Merged
maryia-lapata added a commit that referenced this pull request May 29, 2019
@maryia-lapata
[Vis: Default editor] EUIficate top_sort param editor (#36629) (#37322)
b0e8e5e
jkakavas pushed a commit to jkakavas/kibana that referenced this pull request May 30, 2019
@sulemanof @jkakavas
[Vis: Default editor] EUIficate top_sort param editor (elastic#36629)
a0149f8 
* EUIficate top_sort param editor

* Set field if only one available

* Remove unused translations

* Fix functional tests

* Fix browser tests

* Rewrite functional test for combo_box field select

* Remove unused translations

* Update label
@sulemanof sulemanof deleted the EUIfication/top_sort branch June 5, 2019 08:21
@maryia-lapata maryia-lapata mentioned this pull request Jul 8, 2019
Closed
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Jul 8, 2019
@sulemanof @maryia-lapata
[Vis: Default editor] EUIficate top_sort param editor (elastic#36629)
4c41222 
* EUIficate top_sort param editor

* Set field if only one available

* Remove unused translations

* Fix functional tests

* Fix browser tests

* Rewrite functional test for combo_box field select

* Remove unused translations

* Update label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryia-lapata maryia-lapata maryia-lapata approved these changes

@dmlemeshko dmlemeshko dmlemeshko approved these changes

@wylieconlon wylieconlon wylieconlon approved these changes

@cchaos cchaos cchaos approved these changes

@chrisdavies chrisdavies Awaiting requested review from chrisdavies

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sulemanof @elasticmachine @cchaos @wylieconlon @dmlemeshko @maryia-lapata