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

[Region Map] Fix loading default vector map and base layer setting #43858

Merged
merged 10 commits into from
Sep 11, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Aug 23, 2019

Summary

Fix #38002.

Should set the default value (actually the first one) from the dropdown.
The reason is caused by separate states in the visualization and the editor.
After loading the vis, the editor watcher updates its state with state from the vis ->

src/legacy/ui/public/vis/editors/default/default.js

image

The changes are done with eslint react-hooks plugin enabled, so necessary values were added as watchers

Also crated a useMount custom effect for invoking callbacks only after the first render.

Checklist

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

For maintainers

@sulemanof sulemanof added release_note:fix Feature:Region Map Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0 labels Aug 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof changed the title [Region Map] Fix loading default vector map and vase layer setting [Region Map] Fix loading default vector map and base layer setting Aug 26, 2019
@sulemanof sulemanof requested a review from a team as a code owner August 26, 2019 07:30
@flash1293
Copy link
Contributor

@ppisljar could you take this one? Not completely sure about the implications - is this the same thing we discussed in #38644 ?

@flash1293 flash1293 removed their request for review August 26, 2019 08:36
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof
Copy link
Contributor Author

Hi @timroes @ppisljar !
Could you please take a look at this? Thanks in advance!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes requested review from flash1293 and removed request for timroes and flash1293 August 30, 2019 13:05
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

while this does seem to fix the bug i don't like the way we are calling forceUpateVis()

the real problem seems to be that vis defaults are not set correctly due to the fact that we need to get the list of layers async. We can either make sure the defaults are correctly set, or have a working fallback in the maps visualization.

i am also not sure if using forceUpdateVis() might have some other side effects that we are not aware of here.

@@ -75,10 +80,18 @@ function WmsOptions({

if (!wms.selectedTmsLayer && newBaseLayers.length) {
setWmsOption('selectedTmsLayer', newBaseLayers[0]);
Copy link
Member

Choose a reason for hiding this comment

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

setting this here will require:

  • clicking on the "play" button
  • or calling forceUpdateVis (to send changes down to vis)

however i am wondering, should this happen here ? i see a few options:

  • have tms stuff preloaded (at the vis registration time) ... then we should be able to set visualization defaults to the first layer.
  • move the logic into map visualization .... if layer is not set use the first layer

i would prefer the first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code which registers vis is run every time when the app opens (or the page is refreshed) no matter what plugin opened. I think the layers should be fetched only when map visualization opens. For example during redirecting to the editor but before rendering the default editor we can load tms layers and set the first layer. I tried to implement this idea. @ppisljar could you please have a look?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata merged commit 32d98d5 into elastic:master Sep 11, 2019
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Sep 11, 2019
…lastic#43858)

* Fix loading default vector layer

* Move layers loading to vis initialization

* Move layers loading to editor initialization
maryia-lapata added a commit that referenced this pull request Sep 11, 2019
…43858) (#45363)

* Fix loading default vector layer

* Move layers loading to vis initialization

* Move layers loading to editor initialization
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 11, 2019
…-to-np-ready

* 'master' of github.com:elastic/kibana: (25 commits)
  [ML] Fixes display of matching modules in index data visualizer (elastic#45261)
  [Console] Update indentation behaviour (elastic#45249)
  Convert value provided to PhraseValueInput to string to catch Exception (elastic#45259)
  [Region Map] Fix loading default vector map and base layer setting (elastic#43858)
  [ML] Fixing empty time range when cloning jobs (elastic#45286)
  [ML] Fixing wizard validation delay (elastic#45265)
  [Logs UI] Interpret finished analysis jobs as healthy (elastic#45268)
  [Console] SQL template with triple quote in completion (elastic#45248)
  [ML] Data Frames: Cards as links (elastic#45254)
  fix(code/frontend): should show updating instead of cloning when updating (elastic#45238)
  fix(code/frontend): fix document search result from (elastic#45236)
  disable another flaky suite (elastic#45323) (elastic#45330)
  disable flaky suite (elastic#45105)
  skip flaky suite (elastic#43069)
  skip flaky suite (elastic#45089)
  disable jest suite that has no enabled tests (elastic#44250)
  disable flaky test (elastic#45317)
  disable flaky test (elastic#45315)
  [DOCS] Creates developer folder (elastic#45280)
  [SIEM] Changes ML conditional links to use tabs, fixes a small bug with null filterQuery   (elastic#45218)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 11, 2019
…ditor

* 'master' of github.com:elastic/kibana: (76 commits)
  Upgrade EUI to 13.8.1 (elastic#45052)
  [ML] Add multi metric job wizard test (elastic#45279)
  [SIEM] Inject/apply KQL changed in refresh button (elastic#45065)
  [Graph] Type persistence (elastic#44985)
  Functional tests: convert more test/services to TS (elastic#45176)
  [ML] Fixes display of matching modules in index data visualizer (elastic#45261)
  [Console] Update indentation behaviour (elastic#45249)
  Convert value provided to PhraseValueInput to string to catch Exception (elastic#45259)
  [Region Map] Fix loading default vector map and base layer setting (elastic#43858)
  [ML] Fixing empty time range when cloning jobs (elastic#45286)
  [ML] Fixing wizard validation delay (elastic#45265)
  [Logs UI] Interpret finished analysis jobs as healthy (elastic#45268)
  [Console] SQL template with triple quote in completion (elastic#45248)
  [ML] Data Frames: Cards as links (elastic#45254)
  fix(code/frontend): should show updating instead of cloning when updating (elastic#45238)
  fix(code/frontend): fix document search result from (elastic#45236)
  disable another flaky suite (elastic#45323) (elastic#45330)
  disable flaky suite (elastic#45105)
  skip flaky suite (elastic#43069)
  skip flaky suite (elastic#45089)
  ...
@sulemanof sulemanof deleted the fix/region_map_vector_layer branch September 16, 2019 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back the default selection of world countries for vector maps on region map
5 participants