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_aggregate and size param editors #36567

Merged
merged 16 commits into from
May 29, 2019
Merged

[Vis: Default editor] EUIficate top_aggregate and size param editors #36567

merged 16 commits into from
May 29, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented May 15, 2019 •
edited
Loading

Summary

Part of #30922.
EUIfication of top_aggregate_and_size control for Top Hit aggregation parameter.

no_compatible_aggs

Added new behavior:

  • when there are no compatible aggregations for the chosen field, the select becomes disabled, the icon becomes alert type and the tooltip shows Chosen field has no compatible aggregations

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 15, 2019 13:01
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sulemanof sulemanof changed the title EUIficate top_aggregate and size param editors [Vis: Default editor] EUIficate top_aggregate and size param editors May 15, 2019
@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 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

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

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof force-pushed the EUIfication/top_aggregate_and_size branch from 9ae5a8d to 019cf75 Compare May 17, 2019 09:00
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

maryia-lapata
maryia-lapata approved these 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.

Code LGTM with small comments. I also tested with custom locale and tested with screenreader VoiceOver, it works fine.

src/legacy/ui/public/agg_types/controls/top_aggregate.tsx Outdated Show resolved Hide resolved
src/legacy/ui/public/agg_types/controls/top_aggregate.tsx Outdated Show resolved Hide resolved
src/legacy/ui/public/vis/editors/default/agg_param.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cchaos
cchaos reviewed May 20, 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.

My main comment is that the field errors should not be hidden by a tooltip.

Screen Shot 2019-05-20 at 13 54 25 PM

You should use the EuiFormHelpText component below the fields.

@sulemanof
Copy link
Contributor Author

Info moved to a help text:

image

@cchaos please, take a look 😊

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented May 21, 2019

@sulemanof Is there anyway to disable both the "with" and "size" inputs and then have the help/error text span both columns?

Also, it should actually be a EuiFormErrorText element, so that it gets the danger text color.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof
Copy link
Contributor Author

sulemanof commented May 22, 2019 •
edited
Loading

@cchaos since Aggregate with and Size param editors has not a common parent, I do not see a good way to make EuiFormErrorText under both.
The best option I could propose now is to show the EuiFormErrorText under the field, when it has no compatible aggregation and FMPOV it's even better way, because a user will understand it need to change a field. See the gif attached below:

top_hit

@cchaos
Copy link
Contributor

cchaos commented May 22, 2019

@sulemanof Great solution! I agree that's a much better way to handle it, especially since it's the field that is causing the error, not the "with". I also like that you disabled the size field as well. However, I think they should only be disabled if the selected field is incompatible.

This state should have all controls enabled
Screen Shot 2019-05-22 at 9 16 05 AM

No need to set the "Aggregate with" field as hasErrors
Since it's already errored at the field control.
Screen Shot 2019-05-22 at 9 15 12 AM

But other than those two, nit-picky things, I do really like this solution.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof
Copy link
Contributor Author

@cchaos I agree with validation logic, done!

top_hit

cchaos
cchaos approved these changes May 22, 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.

Great! LGTM. Thanks @sulemanof !

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof
Copy link
Contributor Author

@wylieconlon @chrisdavies please, review 😊

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata merged commit 3665ba0 into elastic:master May 29, 2019
@maryia-lapata maryia-lapata mentioned this pull request May 29, 2019
Merged
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_aggregate and size param editors (e…
e7b6e3a 
…lastic#36567)

* EUIficate top_aggregate_and_size param editor

* Remove template

* Change typescript interfaces

* Fix browser tests

* Add an icon alert

* Changes due to comments

* Move error to a help text

* Move error to a field

* Change validation logic

* Fix discarding changes action

* Remove changed translation
maryia-lapata added a commit that referenced this pull request May 29, 2019
@maryia-lapata
[Vis: Default editor] EUIficate top_aggregate and size param editors (#…
d0a0c3f 
…36567) (#37324)

* EUIficate top_aggregate_and_size param editor

* Remove template

* Change typescript interfaces

* Fix browser tests

* Add an icon alert

* Changes due to comments

* Move error to a help text

* Move error to a field

* Change validation logic

* Fix discarding changes action

* Remove changed translation
jkakavas pushed a commit to jkakavas/kibana that referenced this pull request May 30, 2019
@sulemanof @jkakavas
[Vis: Default editor] EUIficate top_aggregate and size param editors (e…
59d3e0d 
…lastic#36567)

* EUIficate top_aggregate_and_size param editor

* Remove template

* Change typescript interfaces

* Fix browser tests

* Add an icon alert

* Changes due to comments

* Move error to a help text

* Move error to a field

* Change validation logic

* Fix discarding changes action

* Remove changed translation
@sulemanof sulemanof deleted the EUIfication/top_aggregate_and_size branch June 5, 2019 09:32
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

@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.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

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