-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add EMS hot link #21154
Add EMS hot link #21154
Conversation
@@ -21,6 +21,12 @@ | |||
</div> | |||
</div> | |||
|
|||
<div class="kuiSideBarFormRow" ng-hide="!editorState.params.emsHotLink"> | |||
<div class="kuiSideBarFormRow__control"> | |||
Preview <a ng-href="{{editorState.params.emsHotLink}}" target="_">{{editorState.params.selectedLayer.name}} on the Elastic Maps Service</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we flesh out the UI are we expecting this to be a similar UX to the icon-tooltip as seen in some of the other fields on this page? Or does including a link tag make it too complicated for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, this needs to be improved. Icon-tooltip might help (e.g. similar to hot-linking the ES-doc for each bucket-aggregation
💔 Build Failed |
💔 Build Failed |
jenkins, test this |
src/server/config/schema.js
Outdated
emsLandingPageUrl: Joi.when('$dev', { | ||
is: true, | ||
then: Joi.string().default('https://maps-staging.elastic.co/v2'), | ||
otherwise: Joi.string().default('https://maps.elastic.co/v2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @thomasneirynck previously, the current manifest used to populate the dropdown reports slightly different layers than what's available at the current EMS endpoints, for example 'USA Counties' is listed as available via the manifest, but the dynamically generated link with that layer name argument provided doesn't actually go to the resource. This is a known mismatch originally identified by @nickpeihl, and is something we can tackle either as a part of this issue or it could be a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confident this will be a separate issue and doesn't necessarily have to hold up merging for now so long as the external services are updated and we update the manifest used for the dropdown and the generated link. I ran the code locally with no apparent issues. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a separate issue, mainly because I misunderstood how the EMS landing page deployment works. maps-staging.elastic.co/v2 is showing the production manifest rather than the staging manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we decided to just have Kibana use production-EMS all the time, even in dev/master. #21237 addresses that (and should be merged before this one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see input from the design team on how the link appears, but functionally, this is perfect.
@nickpeihl agreed, will run this by design team |
💔 Build Failed |
54c9845
to
e977d73
Compare
💚 Build Succeeded |
The link is a little long-winded. I don't think it's necessary to duplicate the name of the layer in the link, it can just always read "Preview on the EMS" and then add either a tooltip or a I'd then reduce the font-size, which isn't really easily done with generic tags, so I'd just add: .vis-editor-config .ems-hotlink {
font-size: 12px;
} The extra selector is just to ensure scoping But the functionality works and the zooming when you land on the EMS page is a really nice touch! |
thanks for feedback @cchaos, will make the changes.. |
💚 Build Succeeded |
38cf091
to
5d76d1f
Compare
I rebased this with master, since this was blocked by #21237. I also made the corresponding change to always point at the production URL of the landing page. |
💚 Build Succeeded |
Adds a dynamic hyperlink to the EMS landing page for EMS vector layers selected in the vector layer drop down of region maps.
Adds a dynamic hyperlink to the EMS landing page for EMS vector layers selected in the vector layer drop down of region maps.
This adds a dynamic hotlink for the EMS documentation to the EMS landing page for EMS vector layers selected in the vector layer drop down of region maps.
Example (NOTE: these images are
For EMS data:
For non-EMS data (no change):