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

[Maps] Remove unneeded and breaking layer condition that prevents cancel from add layer panel #31634

Merged
merged 8 commits into from
Feb 25, 2019

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Feb 20, 2019

This bool condition appears to be no longer necessary and in fact, causes a bug. Requiring a layer to have been selected prevents cancelling before a layer has been selected. This update removes this condition.

@kindsun kindsun added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Feb 20, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@kindsun kindsun added bug Fixes for quality problems that affect the customer experience v7.0.0 v8.0.0 v6.7.0 v7.2.0 labels Feb 20, 2019
@kindsun kindsun changed the title Remove unneeded and breaking layer condition that prevents cancel from add layer panel [Maps] Remove unneeded and breaking layer condition that prevents cancel from add layer panel Feb 20, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kindsun kindsun requested a review from nreese February 20, 2019 23:51
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

The check for this.state.layer around this.props.removeTransientLayer() can also be removed.

Lets add some functional tests to this PR for some of these flows. Lets add tests that verify

  1. Click Add layer, verify add panel opens
  2. Click Cancel, verify add panel closes
  3. Click Add layer, add source. Save map. Verify transient layer is removed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

functional tests are shaping up nicely. Need to remove the sleeps because they lead to timing issues

// Select layer
const vectorLayer = await PageObjects.maps.selectVectorLayer();
// Confirm layer added
await PageObjects.common.sleep(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

sleeps lead to flaky tests because they are a work around for timing issues that can easily break.

Instead of sleep, verify Add layer button is active and spinner no longer exists. Do something like await addLayerButton.waitForDeletedByClassName('euiLoadingSpinner');

panelOpen = await PageObjects.maps.isLayerAddPanelOpen();
expect(panelOpen).to.be(false);
// Verify layer has been removed
await PageObjects.common.sleep(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of sleep, wrap expect in retry. Sleeps lead to flaky tests because they mask timing issues

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review, tested changes in chrome

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kindsun kindsun merged commit 4ac821d into elastic:master Feb 25, 2019
kindsun added a commit to kindsun/kibana that referenced this pull request Feb 25, 2019
…cel from add layer panel (elastic#31634)

* Remove unneeded and breaking layer condition that prevents cancelling unless a layer's selected

* Add layer tests. Review feedback

* .....and the test file

* Review feedback. Factor out reusable maps test functions. Clean up

* Picky picky

* Review feedback. Remove sleeps

# Conflicts:
#	x-pack/test/functional/apps/maps/index.js
kindsun added a commit to kindsun/kibana that referenced this pull request Feb 25, 2019
…cel from add layer panel (elastic#31634)

* Remove unneeded and breaking layer condition that prevents cancelling unless a layer's selected

* Add layer tests. Review feedback

* .....and the test file

* Review feedback. Factor out reusable maps test functions. Clean up

* Picky picky

* Review feedback. Remove sleeps

# Conflicts:
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/view.js
#	x-pack/test/functional/apps/maps/index.js
kindsun added a commit to kindsun/kibana that referenced this pull request Feb 25, 2019
…cel from add layer panel (elastic#31634)

* Remove unneeded and breaking layer condition that prevents cancelling unless a layer's selected

* Add layer tests. Review feedback

* .....and the test file

* Review feedback. Factor out reusable maps test functions. Clean up

* Picky picky

* Review feedback. Remove sleeps

# Conflicts:
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/view.js
#	x-pack/test/functional/apps/maps/index.js
kindsun added a commit that referenced this pull request Feb 25, 2019
…cel from add layer panel (#31634) (#31926)

* Remove unneeded and breaking layer condition that prevents cancelling unless a layer's selected

* Add layer tests. Review feedback

* .....and the test file

* Review feedback. Factor out reusable maps test functions. Clean up

* Picky picky

* Review feedback. Remove sleeps

# Conflicts:
#	x-pack/test/functional/apps/maps/index.js
kindsun added a commit that referenced this pull request Feb 25, 2019
…cel from add layer panel (#31634) (#31932)

* Remove unneeded and breaking layer condition that prevents cancelling unless a layer's selected

* Add layer tests. Review feedback

* .....and the test file

* Review feedback. Factor out reusable maps test functions. Clean up

* Picky picky

* Review feedback. Remove sleeps

# Conflicts:
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/view.js
#	x-pack/test/functional/apps/maps/index.js
kindsun added a commit that referenced this pull request Feb 25, 2019
…ts cancel from add layer panel (#31634) (#31930)

* [Maps] Remove unneeded and breaking layer condition that prevents cancel from add layer panel (#31634)

* Remove unneeded and breaking layer condition that prevents cancelling unless a layer's selected

* Add layer tests. Review feedback

* .....and the test file

* Review feedback. Factor out reusable maps test functions. Clean up

* Picky picky

* Review feedback. Remove sleeps

# Conflicts:
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/__snapshots__/view.test.js.snap
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js
#	x-pack/plugins/maps/public/components/widget_overlay/layer_control/view.js
#	x-pack/test/functional/apps/maps/index.js

* Remove trailing space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants