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.

Sign up for GitHub

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

Jump to bottom

[Maps] Add hiddenLayers option to embeddable map input #54355

Merged
merged 3 commits into from
Jan 10, 2020
Merged

[Maps] Add hiddenLayers option to embeddable map input #54355

merged 3 commits into from
Jan 10, 2020

Conversation

crob611
Copy link
Contributor

@crob611 crob611 commented Jan 9, 2020

Summary

This PR allows a MapEmbeddable to accept a new key on its input hiddenLayers. This array of ids are used to set the initial visibility of all of the maps layers.

The use case for this is in Canvas, when a map is embedded and layers are made visible or invisible, we want capture those state changes in the expression backing the embed, so that the map can be configured and remain consistent on reloading of the workpad.

This PR only includes the changes to map embeddable, but if you would like to see it in action, you can check out this branch which integrates this with canvas so that toggling layers will be represented in the expression https://github.com/crob611/kibana/tree/map-embeddable-hidden-layers

@crob611 crob611 requested a review from a team as a code owner January 9, 2020 15:28
@crob611 crob611 added review v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 9, 2020
nreese
nreese reviewed Jan 9, 2020
View reviewed changes
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.

Please update map embeddable input documentation for the new parameter at https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/public/embeddable/README.md

x-pack/legacy/plugins/maps/public/embeddable/map_embeddable.js Outdated
@@ -244,5 +257,17 @@ export class MapEmbeddable extends Embeddable {
openTOCDetails,
});
}

const hiddenLayerIds = this._store
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this logic to a selector in https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/public/selectors/map_selectors.js

x-pack/legacy/plugins/maps/public/embeddable/map_embeddable.js Show resolved Hide resolved
@crob611
Copy link
Contributor Author

crob611 commented Jan 9, 2020

@nreese updated with changes. Let me know specifically if you think I'm handling the waiting_for_map_ready vs mapReady scenario correctly.

nreese
nreese reviewed Jan 10, 2020
View reviewed changes
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.

Thanks for creating actions and selectors. Can you please remove the now unused action TOGGLE_LAYER_VISIBLE?

x-pack/legacy/plugins/maps/public/selectors/map_selectors.js Outdated
@@ -150,6 +150,8 @@ export const getLayerList = createSelector(
}
);

export const getHiddenLayers = state => getLayerListRaw(state).filter(layer => !layer.visible);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use reselect for this selector definition since it needs to derive its state from getLayerListRaw?

export const getHiddenLayers = createSelector(
  getLayerListRaw,
  (layerDescriptorList) => {
    return layerDescriptorList.filter(layer => !layer.visible);
  }
);

x-pack/legacy/plugins/maps/public/embeddable/map_embeddable.js Outdated
@@ -244,5 +248,15 @@ export class MapEmbeddable extends Embeddable {
openTOCDetails,
});
}

const hiddenLayerIds = getHiddenLayers(this._store.getState())
.map(layer => layer.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move .map and .sort to the selector? The selector should return the data in the format needed here.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

nreese
nreese approved these changes Jan 10, 2020
View reviewed changes
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.

Thanks for making all the changes

lgtm
code review

@crob611 crob611 merged commit 934d6b3 into elastic:master Jan 10, 2020
@crob611 crob611 mentioned this pull request Jan 10, 2020
Merged
crob611 pushed a commit to crob611/kibana that referenced this pull request Jan 10, 2020
[Maps] Add hiddenLayers option to embeddable map input (elastic#54355)
0eac340 
* Add hiddenLayers option to embeddable map input

* Move hiddenLayers logic to actions and reducers. Adds Documentation

* Address code review suggestions
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 13, 2020
@gmmorris
Merge branch 'master' into task-manager/schema-config
4173614 
* master: (69 commits)
  [Graph] Fix various a11y issues (elastic#54097)
  Add ApplicationService app status management (elastic#50223)
  logs in one time (elastic#54447)
  Deprecate using `elasticsearch.ssl.certificate` without `elasticsearch.ssl.key` and vice versa (elastic#54392)
  [Optimizer] Fix a stack overflow with watch_cache when it attempts to delete very large folders. (elastic#54457)
  Security - Role Mappings UI (elastic#53620)
  [SIEM] [Detection engine] Permission II (elastic#54292)
  Allow User to Cleanup Repository from UI  (elastic#53047)
  [Detection engine] Some UX for rule creation (elastic#54471)
  share specific instances of some ui packages (elastic#54079)
  [ML] APM modules configs for RUM Javascript and NodeJS (elastic#53792)
  [APM] Delay rendering invalid license notification (elastic#53924)
  [Graph] Improve error message on graph requests (elastic#54230)
  [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (elastic#53719)
  Unit Tests for common/lib (elastic#53736)
  [Graph] Only show explorable fields (elastic#54101)
  remove linting rule exception for markdown (elastic#54232)
  [Monitoring] Fetch shard data more efficiently (elastic#54028)
  [Maps] Add hiddenLayers option to embeddable map input (elastic#54355)
  Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function (elastic#54391)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 13, 2020
@gmmorris
Merge branch 'master' into alerting/zombie-redux
0ce6ed5 
* master: (69 commits)
  [Graph] Fix various a11y issues (elastic#54097)
  Add ApplicationService app status management (elastic#50223)
  logs in one time (elastic#54447)
  Deprecate using `elasticsearch.ssl.certificate` without `elasticsearch.ssl.key` and vice versa (elastic#54392)
  [Optimizer] Fix a stack overflow with watch_cache when it attempts to delete very large folders. (elastic#54457)
  Security - Role Mappings UI (elastic#53620)
  [SIEM] [Detection engine] Permission II (elastic#54292)
  Allow User to Cleanup Repository from UI  (elastic#53047)
  [Detection engine] Some UX for rule creation (elastic#54471)
  share specific instances of some ui packages (elastic#54079)
  [ML] APM modules configs for RUM Javascript and NodeJS (elastic#53792)
  [APM] Delay rendering invalid license notification (elastic#53924)
  [Graph] Improve error message on graph requests (elastic#54230)
  [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (elastic#53719)
  Unit Tests for common/lib (elastic#53736)
  [Graph] Only show explorable fields (elastic#54101)
  remove linting rule exception for markdown (elastic#54232)
  [Monitoring] Fetch shard data more efficiently (elastic#54028)
  [Maps] Add hiddenLayers option to embeddable map input (elastic#54355)
  Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function (elastic#54391)
  ...
crob611 pushed a commit that referenced this pull request Jan 13, 2020
@elasticmachine
[Maps] Add hiddenLayers option to embeddable map input (#54355) (#54485)
105c452 
* Add hiddenLayers option to embeddable map input

* Move hiddenLayers logic to actions and reducers. Adds Documentation

* Address code review suggestions

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes review v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@crob611 @kibanamachine @nreese