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 tag cloud options tab #42070

Merged
merged 17 commits into from
Jul 31, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jul 26, 2019

Summary

A part of #38273.
EUIfication of the Options tab in the Tag cloud vis.

tag_opts

This includes:

  • reused the ValidatedDualRange component with existing validation (it's used in Maps plugin), the component is also extended with additional props;
  • capitalized and made wrapped in i18n all labels in select options in tag cloud config collections and pie chart config collections;
  • removed no-ui-slider lib, since it was only used there;

Checklist

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

For maintainers

@sulemanof sulemanof requested a review from a team as a code owner July 26, 2019 14:10
@sulemanof sulemanof changed the title [Vis: Default editor] EUIficate tag cloud options tab [WIP][Vis: Default editor] EUIficate tag cloud options tab Jul 26, 2019
@sulemanof sulemanof added the release_note:skip Skip the PR/issue when compiling release notes label Jul 26, 2019
@sulemanof sulemanof requested a review from cchaos July 26, 2019 14:55
@sulemanof
Copy link
Contributor Author

@cchaos could you please take a look ? Maybe you have any suggestions

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Jul 26, 2019

Looking at the screenshot, I'd say put the unit of measure (pixels) in the form label and remove the labels.

@sulemanof sulemanof changed the title [WIP][Vis: Default editor] EUIficate tag cloud options tab [Vis: Default editor] EUIficate tag cloud options tab Jul 29, 2019
@sulemanof sulemanof requested a review from maryia-lapata July 29, 2019 11:03
@sulemanof
Copy link
Contributor Author

sulemanof commented Jul 29, 2019

Looking at the screenshot, I'd say put the unit of measure (pixels) in the form label and remove the labels.

@cchaos thanks !
The PR is updated due to changes request and is ready for review!

@sulemanof sulemanof added Feature:Vis Editor Visualization editor issues Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0 labels Jul 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sulemanof
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Screenshots and code LGTM

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.

LGTM, just small comments.
checked locally on Chrome, Mac. It works as expected.

@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.

Code LGTM, tested and works.

This is unrelated, but adding the label overflows some container an a scroll bar shows up:
Screenshot 2019-07-31 at 10 22 57

Maybe due to an earlier EUIfication?

package.json Show resolved Hide resolved
@@ -0,0 +1,85 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe use tag_cloud_options.tsx as name here to match the component name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.
Also the components folder created for the future react components

"tagCloud.visParams.maxFontSizeAriaLabel": "标记字体最大大小",
"tagCloud.visParams.maxPixelsAriaLabel": "像素",
"tagCloud.visParams.minFontSizeAriaLabel": "标记字体最小大小",
"tagCloud.visParams.minPixelsAriaLabel": "像素",
"tagCloud.visParams.orientationsLabel": "方向",
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason the orientations label was spared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason of removing other labels that they were not in sentence case (we try to follow EUI writing style guide) or labels become unnecessary, or they were changed.
Here the tagCloud.visParams.orientationsLabel stayed the same as it was -> so there was not a reason to remove it

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof
Copy link
Contributor Author

sulemanof commented Jul 31, 2019

Code LGTM, tested and works.

This is unrelated, but adding the label overflows some container an a scroll bar shows up:

Maybe due to an earlier EUIfication?

@flash1293 thanks Joe 👍
Since it is unrelated to the PR and FMPOV has low priority, I mark the issue and will check it later

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof merged commit 48921a8 into elastic:master Jul 31, 2019
@sulemanof sulemanof deleted the EUIfication/options/tag_cloud branch July 31, 2019 12:11
sulemanof added a commit to sulemanof/kibana that referenced this pull request Jul 31, 2019
* EUIficate pie chart options tab

* Fix tests

* Size titles down to xs

* Use FormattedMessage

* EUIficate tag_cloud_vis options tab

* Get rid of ticks in the range

* Update snapshots

* Fix comments

* Change label

* Create a components folder
sulemanof added a commit that referenced this pull request Jul 31, 2019
* EUIficate pie chart options tab

* Fix tests

* Size titles down to xs

* Use FormattedMessage

* EUIficate tag_cloud_vis options tab

* Get rid of ticks in the range

* Update snapshots

* Fix comments

* Change label

* Create a components folder
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.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants