[Maps] move source details to Panel header #29298

nreese · 2019-01-24T23:23:05Z

Follow up to #28716.

Moves source details to Panel header. Source details are no longer rendered by the source. Now the source just provides a list of properties that are rendered by the layer_panel view. Also, the PR takes careful concern to ensure there is symmetry between the layer_addpanel inputs and then viewed source details so the name of the properties is the same and their is correlation between creating a source and viewing the details

PR also fixes the EMS vector layer link. In master, it is getting rendered as a unresolved promise. Now that properties are loaded async so the promise can resolve

elasticmachine · 2019-01-24T23:25:34Z

Pinging @elastic/kibana-gis

nreese · 2019-01-24T23:25:40Z

cc @cchaos

thomasneirynck · 2019-01-24T23:35:24Z

x-pack/plugins/gis/public/components/layer_panel/settings_panel/index.js

@@ -23,7 +23,6 @@ function mapStateToProps(state = {}) {
    layerId: selectedLayer.getId(),
    maxZoom: selectedLayer.getMaxZoom(),
    minZoom: selectedLayer.getMinZoom(),
-    renderSourceDetails: selectedLayer.renderSourceDetails,


should be careful for a couple small merge conflicts with https://github.com/elastic/kibana/pull/29294/files

thomasneirynck · 2019-01-24T23:36:05Z

x-pack/plugins/gis/public/components/layer_panel/settings_panel/settings_panel.js

@@ -4,7 +4,7 @@
 * you may not use this file except in compliance with the Elastic License.


ugh, I absolute hate that function syntax for a react component. so cluttered.

But it is more performant since there are not state checks or life cycle methods

interesting. but if we want to avoid React internally to do this whole life-cycle loop (since internally, it does go through it even when using function-components), we would also need to re-implement how we include the component.

I just got this from:
https://medium.com/missive-app/45-faster-react-functional-components-now-3509a668e69f

nreese · 2019-01-24T23:39:28Z

PR also adds icons for WMS and TMS layers

…t mounts

elasticmachine · 2019-01-25T00:40:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2019-01-25T01:42:50Z

flaky test

jenkins, test this

elasticmachine · 2019-01-25T02:24:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2019-01-25T02:49:58Z

flaky test

jenkins, test this

elasticmachine · 2019-01-25T03:28:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…el input

elasticmachine · 2019-01-25T13:50:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-25T20:57:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cchaos · 2019-01-25T21:06:26Z

x-pack/plugins/gis/public/shared/layers/sources/ems_file_source.js

-    return (
-      <EuiText color="subdued" size="s">
-        <p className="gisLayerDetails">
-          <strong className="gisLayerDetails__label">Source </strong><span>Elastic Maps Service</span><br/>


Can you not use the same formatting as I had added here in the new location?

yes. Same components just rendered with different data.

cchaos · 2019-01-25T21:13:44Z

Some design feedback:

Line up accordion with layer title

Use EuiText size="xs" for the details
Add spacing after accordion header
Remove spacing between detail items

thomasneirynck · 2019-01-25T23:50:18Z

x-pack/plugins/gis/public/components/layer_panel/settings_panel/settings_panel.js


-        {this.renderZoomSliders()}
+      {props.renderSourceSettingsEditor({ onChange: onSourceChange })}


after merging this will become props.layer.renderSourceSettingsEditor().

thomasneirynck · 2019-01-25T23:50:56Z

x-pack/plugins/gis/public/shared/layers/layer.js

-  renderSourceDetails = () => {
-    return this._source.renderDetails();
-  };
+  async getImmutableSourceProperties() {


nice rename

thomasneirynck · 2019-01-25T23:51:36Z

x-pack/plugins/gis/public/shared/layers/sources/es_geohashgrid_source/es_geohashgrid_source.js

+      // ignore error, title will just default to id
+    }
+
+    return [


nice improvement to get consistent formatting.

the only thing that might bite us if we want to do more fancy stuff depending on the source-type. E.g. ems-hotlinking is such a small edge-case. I feel like we take a step back with this:

It used to say "Preview on Elastic Maps Landing Page", which is more descriptive

thomasneirynck

I'm good with it, but feel like we're taking a small step back with regard to adding more customized source details. e.g. the ems hotlink is less descriptive.

thomasneirynck · 2019-01-25T23:54:10Z

x-pack/plugins/gis/public/shared/layers/sources/es_geohashgrid_source/es_geohashgrid_source.js

+      // ignore error, title will just default to id
+    }
+
+    return [


the only thing that might bite us if we want to do more fancy stuff depending on the source-type. E.g. ems-hotlinking is such a small edge-case. I feel like we take a step back with this:

It used to say "Preview on Elastic Maps Landing Page", which is more descriptive

nreese · 2019-01-26T19:42:05Z

@cchaos I updated the styling as requested and now it looks like. Let me know if I butchered the CSS changes

elasticmachine · 2019-01-26T20:27:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cchaos

Thanks for the update. I think we can just get rid of the extra classes and associated styles for the .gisLayerDetails__label and similar. It just doesn't work with such unknown/dynamic content.

cchaos · 2019-01-28T16:16:28Z

x-pack/plugins/gis/public/components/layer_panel/_layer_panel.scss

+}
+
+.gisLayerDetails {
+  p {


Naked selectors are frowned upon in our linter. You'll want to add a class to the actual <p> tags.

cchaos · 2019-01-28T16:18:39Z

x-pack/plugins/gis/public/components/layer_panel/view.js

+      }
+      return (
+        <p key={label}>
+          <strong className="gisLayerDetails__label">{label}</strong>


You can remove this class and remove the span around the value since I was trying to do some line-up trickery but it looks like it won't work in most cases including the screenshot you've got. So just let them line-up normally.

cchaos · 2019-01-28T16:18:54Z

x-pack/plugins/gis/public/components/layer_panel/view.js

+              id="accordion1"
+              buttonContent="Source details"
+            >
+              <EuiText color="subdued" size="s">


I'd suggest making this size="xs"

cchaos · 2019-01-28T16:20:29Z

x-pack/plugins/gis/public/components/layer_panel/view.js

+              buttonContent="Source details"
+            >
+              <EuiText color="subdued" size="s">
+                <div className="gisLayerDetails">


This class (and probably the whole div) can be removed.

cchaos · 2019-01-28T16:24:39Z

x-pack/plugins/gis/public/components/layer_panel/view.js

@@ -81,6 +135,19 @@ export class LayerPanel  extends React.Component {
              </EuiTitle>
            </EuiFlexItem>
          </EuiFlexGroup>
+          <div className="gisLayerPanel__sourceDetails">


I'd add a small or xsmall space before the toggle.

nreese · 2019-01-29T01:43:20Z

@cchaos Made the suggested changes and here is how things look now

elasticmachine · 2019-01-29T02:25:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-29T03:22:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cchaos

Much better, thank you. I'm wondering if there needs to be a colon or something between the terms? Maybe ask @gchaps ?

nreese · 2019-01-29T14:58:15Z

Last I checked with @gchaps, she was against colons but we can always add them if needed.

* [Maps] move source details to Panel header * fix EMS file link * add grid icon to WMS and TMS source * ensure loadingDisplayName and props is only called once when component mounts * ensure display name gets updated when user changes it in settings panel input * clean-up styling * review feedback

gchaps · 2019-01-29T16:06:44Z

@nreese You're right, in general, I don't like colons in the UI. But in this case I think they would help with comprehension:

Data source: Elasticsearch geohash aggregation
Index pattern: kibana_sample_data_logs
Geospatial field: geo.coordinates
Show as: heatmap

If we were to do something like this, then I don't think they would be needed.

[Maps] move source details to Panel header

6df0774

nreese added v7.0.0 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 labels Jan 24, 2019

nreese requested a review from thomasneirynck January 24, 2019 23:26

fix EMS file link

76fb38c

thomasneirynck reviewed Jan 24, 2019

View reviewed changes

nreese added 2 commits January 24, 2019 16:39

add grid icon to WMS and TMS source

abdce9f

ensure loadingDisplayName and props is only called once when componen…

3829f6b

…t mounts

nreese added 2 commits January 25, 2019 05:58

Merge branch 'master' of github.com:elastic/kibana into source_details

4f855e0

ensure display name gets updated when user changes it in settings pan…

f9250d2

…el input

merge with master

e17e4dd

cchaos reviewed Jan 25, 2019

View reviewed changes

thomasneirynck reviewed Jan 25, 2019

View reviewed changes

clean-up styling

b480b91

nreese requested a review from a team as a code owner January 26, 2019 19:41

cchaos reviewed Jan 28, 2019

View reviewed changes

nreese added 2 commits January 28, 2019 18:27

merge with master

16bd9b8

review feedback

cc6b793

merge with master

6790746

cchaos approved these changes Jan 29, 2019

View reviewed changes

thomasneirynck approved these changes Jan 29, 2019

View reviewed changes

nreese merged commit f3046de into elastic:master Jan 29, 2019

nreese mentioned this pull request Jan 29, 2019

[6.x] [Maps] move source details to Panel header (#29298) #29508

Merged

		@@ -4,7 +4,7 @@
		* you may not use this file except in compliance with the Elastic License.


		{this.renderZoomSliders()}
		{props.renderSourceSettingsEditor({ onChange: onSourceChange })}

[Maps] move source details to Panel header #29298

[Maps] move source details to Panel header #29298

Conversation

nreese commented Jan 24, 2019 • edited Loading

elasticmachine commented Jan 24, 2019

nreese commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

nreese commented Jan 24, 2019

elasticmachine commented Jan 25, 2019

💔 Build Failed

nreese commented Jan 25, 2019

elasticmachine commented Jan 25, 2019

💔 Build Failed

nreese commented Jan 25, 2019

elasticmachine commented Jan 25, 2019

💔 Build Failed

elasticmachine commented Jan 25, 2019

💚 Build Succeeded

elasticmachine commented Jan 25, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented Jan 26, 2019

elasticmachine commented Jan 26, 2019

💚 Build Succeeded

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented Jan 29, 2019

elasticmachine commented Jan 29, 2019

💚 Build Succeeded

elasticmachine commented Jan 29, 2019

💚 Build Succeeded

cchaos left a comment

Choose a reason for hiding this comment

nreese commented Jan 29, 2019

gchaps commented Jan 29, 2019

nreese commented Jan 24, 2019 •

edited

Loading

thomasneirynck Jan 25, 2019 •

edited

Loading

cchaos commented Jan 25, 2019 •

edited

Loading