[Vis: Default editor] EUIficate region map options tab #42944

sulemanof · 2019-08-08T14:57:24Z

Summary

A part of #38273.
EUIfication of the Options tab in the Region map vis.

This also fix #36017.

Checklist

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

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

# Conflicts: # src/legacy/core_plugins/kbn_vislib_vis_types/public/controls/select.tsx # src/legacy/core_plugins/kbn_vislib_vis_types/public/utils/with_injected_dependencies.tsx # src/legacy/core_plugins/tile_map/public/components/wms_options.tsx

…ns/region_map_options # Conflicts: # src/legacy/core_plugins/region_map/public/region_map_vis_params.js # src/legacy/core_plugins/region_map/public/shim/legacy_dependencies_plugin.ts # src/legacy/core_plugins/region_map/public/types.ts

elasticmachine · 2019-08-08T14:58:19Z

Pinging @elastic/kibana-app

elasticmachine · 2019-08-08T18:13:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ns/region_map_options

elasticmachine · 2019-08-09T11:16:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-09T11:48:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

src/legacy/core_plugins/region_map/index.ts

src/legacy/core_plugins/region_map/public/region_map_type.js

elasticmachine · 2019-08-12T15:43:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

maryia-lapata

LGTM, tested on Chrome Mac.

Please don't forget about refactoring from #42944 (review)

src/legacy/core_plugins/region_map/public/components/region_map_options.tsx

…ns/region_map_options

elasticmachine · 2019-08-14T09:52:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-08-14T16:04:27Z

I'm not sure about the default value in selects in the region map options - it looks like you can't go back to the empty state once you've chosen a vector map:

flash1293

Code mostly LGTM with two small nits/hints. I think it makes sense to revisit the possible usability issue in the gif above - it just feels strange for the user to have a one-way path in the UI they can't get back from IMHO. Maybe it was discussed already somewhere else.

flash1293 · 2019-08-14T16:06:41Z

src/legacy/core_plugins/region_map/public/components/region_map_options.tsx

+        setEmsHotLink(newLayer);
+      }
+    },
+    [vectorLayers]


nit: should also include setValue

Currently we build the logic with static setValue prop, but it could be changed in the future.. so it would make sense to watch it also. Thnx!

flash1293 · 2019-08-14T16:11:45Z

src/legacy/core_plugins/region_map/public/util.ts

-  return clonedLayer;
-}
+export const mapToLayerWithId = (prefix: string, layer: FileLayer): VectorLayer => ({
+  ...layer,


The removal of cloneDeep means that the FileLayerField objects and the array of those objects in the layer will reference the original ones. Are we sure they are not mutated somewhere? This could cause some bugs that are hard to track down.

It might be totally fine, just to make sure as I'm not familiar with the control flow in that part of the code.

I've checked the code flow and these values are read-only. Also FileLayer array comes straight from the server, so they are safe for modifying.
Of course I could keep the cloneDeep here, but didn't see a necessity in it and skip for a performance boost.

sulemanof · 2019-08-15T08:49:14Z

I'm not sure about the default value in selects in the region map options - it looks like you can't go back to the empty state once you've chosen a vector map:

Hey, @flash1293 !
Actually, we can't have here an empty value, because it would be an unnecessary vis (without any values on the map). The vis works on mapping the data from elasticsearch to the selected layer data.

Also, there is the logic with setting the default value from this dropdown (the first availabale) after initialization. But currently it doesn't work as expected due to existing issue. It caused by inner visualization building logic and does't depend on this PR, so I will prepare a fix for it in a separate PR. After the fix we won't have empty value in the dropdown.

…ns/region_map_options # Conflicts: # src/legacy/core_plugins/kbn_vislib_vis_types/public/controls/select.tsx

flash1293

LGTM in this case, thanks for your explanation!

dmlemeshko

I think we can get rid of sleeps and use innerHtml to get options text in the functional tests

test/functional/apps/visualize/_region_map.js

elasticmachine · 2019-08-15T10:41:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ns/region_map_options

dmlemeshko

LGTM

elasticmachine · 2019-08-15T16:37:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* EUIficate region_map_options * Reuse types * Remove wms_options directive * Remove style import * Fix issue with join field default value # Conflicts: # src/legacy/core_plugins/region_map/public/region_map_vis_params.html

sulemanof added 5 commits August 7, 2019 11:20

EUIficate region_map_options

0053254

Euificate region_map_options

b86a25f

Reuse types

52f9c51

sulemanof requested a review from a team as a code owner August 8, 2019 14:57

sulemanof added Feature:Region Map 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 labels Aug 8, 2019

sulemanof requested review from maryia-lapata and cchaos August 8, 2019 14:59

Fix typo

7792e57

sulemanof added 3 commits August 9, 2019 11:14

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

763e69c

…ns/region_map_options

Remove wms_options directive

34519cc

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

77a903c

…ns/region_map_options

sulemanof force-pushed the EUIfication/options/region_map_options branch from 48cccc2 to 77a903c Compare August 9, 2019 10:06

Remove style import

77de0cb

sulemanof added 2 commits August 9, 2019 15:44

Fix failing tests

1ca3b39

Fix issue with join field default value

cc3a5a5

sulemanof commented Aug 9, 2019

View reviewed changes

src/legacy/core_plugins/region_map/index.ts Show resolved Hide resolved

sulemanof commented Aug 9, 2019

View reviewed changes

src/legacy/core_plugins/region_map/public/region_map_type.js Outdated Show resolved Hide resolved

sulemanof requested a review from timroes August 9, 2019 13:23

Add fullWidth to the number_input

4871b4e

maryia-lapata requested a review from dmlemeshko August 13, 2019 09:08

maryia-lapata approved these changes Aug 13, 2019

View reviewed changes

src/legacy/core_plugins/region_map/public/components/region_map_options.tsx Outdated Show resolved Hide resolved

sulemanof requested review from flash1293, wylieconlon and chrisdavies August 13, 2019 09:28

sulemanof added 2 commits August 14, 2019 11:17

Fix comments

1323b9c

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

0d600ca

…ns/region_map_options

timroes mentioned this pull request Aug 14, 2019

EUIficate visualization option panels #38273

Closed

10 tasks

sulemanof requested review from nreese and thomasneirynck August 14, 2019 15:19

flash1293 reviewed Aug 14, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

ffe7904

…ns/region_map_options # Conflicts: # src/legacy/core_plugins/kbn_vislib_vis_types/public/controls/select.tsx

flash1293 approved these changes Aug 15, 2019

View reviewed changes

Watch setValue in useCallback

edb4e57

dmlemeshko requested changes Aug 15, 2019

View reviewed changes

test/functional/apps/visualize/_region_map.js Outdated Show resolved Hide resolved

test/functional/apps/visualize/_region_map.js Outdated Show resolved Hide resolved

test/functional/apps/visualize/_region_map.js Outdated Show resolved Hide resolved

sulemanof added 2 commits August 15, 2019 18:33

Change functional tests

bac218a

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

97b5dee

…ns/region_map_options

sulemanof requested a review from dmlemeshko August 15, 2019 15:35

dmlemeshko approved these changes Aug 15, 2019

View reviewed changes

sulemanof merged commit df72f91 into elastic:master Aug 15, 2019

sulemanof deleted the EUIfication/options/region_map_options branch August 15, 2019 18:09

sulemanof mentioned this pull request Aug 15, 2019

[7.x] [Vis: Default editor] EUIficate region map options tab (#42944) #43393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vis: Default editor] EUIficate region map options tab #42944

[Vis: Default editor] EUIficate region map options tab #42944

sulemanof commented Aug 8, 2019 •

edited

Loading

elasticmachine commented Aug 8, 2019

elasticmachine commented Aug 8, 2019

elasticmachine commented Aug 9, 2019

elasticmachine commented Aug 9, 2019

elasticmachine commented Aug 12, 2019

maryia-lapata left a comment

elasticmachine commented Aug 14, 2019

flash1293 commented Aug 14, 2019

flash1293 left a comment

flash1293 Aug 14, 2019

sulemanof Aug 15, 2019

flash1293 Aug 14, 2019

sulemanof Aug 15, 2019 •

edited

Loading

sulemanof commented Aug 15, 2019

flash1293 left a comment

dmlemeshko left a comment

elasticmachine commented Aug 15, 2019

dmlemeshko left a comment

elasticmachine commented Aug 15, 2019

[Vis: Default editor] EUIficate region map options tab #42944

[Vis: Default editor] EUIficate region map options tab #42944

Conversation

sulemanof commented Aug 8, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 8, 2019

elasticmachine commented Aug 8, 2019

💔 Build Failed

elasticmachine commented Aug 9, 2019

💔 Build Failed

elasticmachine commented Aug 9, 2019

💔 Build Failed

elasticmachine commented Aug 12, 2019

💚 Build Succeeded

maryia-lapata left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 14, 2019

💚 Build Succeeded

flash1293 commented Aug 14, 2019

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Aug 14, 2019

Choose a reason for hiding this comment

sulemanof Aug 15, 2019

Choose a reason for hiding this comment

flash1293 Aug 14, 2019

Choose a reason for hiding this comment

sulemanof Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

sulemanof commented Aug 15, 2019

flash1293 left a comment

Choose a reason for hiding this comment

dmlemeshko left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

dmlemeshko left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

sulemanof commented Aug 8, 2019 •

edited

Loading

sulemanof Aug 15, 2019 •

edited

Loading