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] Euification of Metric options tab #46930

Merged
merged 35 commits into from
Oct 8, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Sep 30, 2019

Summary

A part of #38273.
EUIfication of the Options tab in the Metric visualization.

image

Applied changes:

  • disabling instead of removing Use color for and Color schema params;
  • removed inequality.js directives;
  • removed unused styles from sidebar.scss;

Checklist

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

For maintainers

sulemanof and others added 28 commits September 16, 2019 14:19
…ns/heatmap

# Conflicts:
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/components/common/number_input.tsx
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/components/options/gauge/ranges_panel.tsx
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/components/options/index.ts
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/types.ts
#	src/legacy/ui/public/vis/vis_types/vislib_vis_type.js
…ns/heatmap

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
…ns/heatmap

# Conflicts:
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/gauge.js
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/goal.js
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/heatmap.js
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/pie.js
#	src/legacy/ui/public/vis/vis_types/vislib_vis_type.js
…ns/heatmap

# Conflicts:
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/controls/heatmap_options.html
…ns/heatmap

# Conflicts:
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/components/options/metrics_axes/label_options.tsx
…ns/metrics

# Conflicts:
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/components/common/index.ts
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/gauge.d.ts
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/gauge.js
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/goal.js
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/kbn_vislib_vis_types.js
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/types.ts
#	src/legacy/core_plugins/kbn_vislib_vis_types/public/utils/collections.ts
#	src/legacy/core_plugins/kibana/public/discover/components/field_chooser/discover_index_pattern_directive.ts
#	src/legacy/ui/public/vis/editors/default/controls/ranges.tsx
@sulemanof sulemanof added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0 labels Oct 1, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@sulemanof sulemanof added the release_note:skip Skip the PR/issue when compiling release notes label Oct 1, 2019
@flash1293 flash1293 self-requested a review October 1, 2019 15:04
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Screenshot 2019-10-02 at 11 33 21

Is it necessary to always show the value in the little popover if the input is also shown?

Screenshot 2019-10-02 at 11 36 14

There is no legend in the metrics vis. Should we hide the help text here?

I'm not sure about the separation of the panels - why is "percentage mode" part of "Ranges"?
Suggestion:
First panel "Settings", contains "Percentage mode" and "Show title"
Second panel "Ranges", contains ranges and color settings (get rid of "Color options" heading, like in Gauge options panel)
Third panel "Style", contains just the font size

@sulemanof
Copy link
Contributor Author

Thanks @flash1293 , your proposal really makes sense!
But I think we still need @cchaos approval to implement it in a such way!

@snide
Copy link
Contributor

snide commented Oct 2, 2019

She's out on vacation so I'll fill in. I think @flash1293's suggestions make sense as well, so you're good from design. You can remove the tooltip by removing the showValue prop.

@sulemanof
Copy link
Contributor Author

Hey @flash1293 and @snide !
Since I haven't had permissions to update the reviewers list (I can't even re-request review),
could you please take a look at this?

Thanks in advance!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293
Copy link
Contributor

flash1293 commented Oct 7, 2019

The changes look good to me.

Not sure whether we should tackle this in this PR, but IMHO there is a lot of UI around ranges that are not used as long as "None" is selected - it feels weird that you can select ranges and then do nothing with them - why selecting them in the first place?

My suggestion would be: There is a switch whether ranges are enabled or not - if it's set to off, none of the rest of the ranges UI is shown. If it is activated, the color is used for the labels initially and you can switch to background, but there is no setting for "none" (that's what the switch is for).

What do you think @sulemanof @snide ?

@sulemanof
Copy link
Contributor Author

sulemanof commented Oct 8, 2019

My suggestion would be: There is a switch whether ranges are enabled or not - if it's set to off, none of the rest of the ranges UI is shown. If it is activated, the color is used for the labels initially and you can switch to background, but there is no setting for "none" (that's what the switch is for).

Hey @flash1293 ! I suppose it could be a great solution, but I think it should be discussed with more people from kibana side. Would it be better to create a separate issue with discuss label and involve there others?

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.

@sulemanof you are probably right. Let's leave the decision to design whether we merge as is here and do this in a separate PR.

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Overall, the new form largely follows the old form although it does do away with the accordions. Along with that, there are several design small visual tweaks we would likely want to make here, so let's do a follow-up PR, regardless. I'll add an approval under the assumption that we'll do some more tidying up.

@sulemanof sulemanof merged commit b25df01 into elastic:master Oct 8, 2019
@sulemanof sulemanof deleted the EUIfiaction/options/metrics branch October 8, 2019 20:37
sulemanof added a commit to sulemanof/kibana that referenced this pull request Oct 8, 2019
* EUIfication of metric_vis_options

* Small fixes

* Remove unused translations

* Fix style comments

* Remove unused styles
sulemanof added a commit that referenced this pull request Oct 9, 2019
)

* EUIfication of metric_vis_options

* Small fixes

* Remove unused translations

* Fix style comments

* Remove unused styles
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Oct 9, 2019
… (elastic#47630)

* EUIfication of metric_vis_options

* Small fixes

* Remove unused translations

* Fix style comments

* Remove unused styles
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.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants