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

Datahub: Fix display of ESRI data on map #652

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Datahub: Fix display of ESRI data on map #652

merged 2 commits into from
Oct 19, 2023

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented Oct 18, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2023

Affected libs: feature-dataviz, feature-record, feature-router, ui-elements, feature-catalog, feature-search, feature-map, ui-catalog, ui-search,
Affected apps: metadata-editor, datahub, demo, webcomponents, search, map-viewer,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@fgravin fgravin added this to the 2.0.1 milestone Oct 18, 2023
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks Tobias, it looks good but I think the method getDataset could be reviewed and improved a bit.
Thanks

@@ -197,7 +197,10 @@ export class DataService {
link.type === 'service' &&
link.accessServiceProtocol === 'esriRest'
) {
const url = this.getDownloadUrlFromEsriRest(linkUrl, 'geojson')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the linkUrl was not explicit enough...
Should be linkProxifiedUrl, could have help to know to not use it, as getDownloadUrlFromEsriRest also proxifies the url.

By the way, I also see that in the same function, the call of getDownloadUrlsFromWfs also proxifies the URL twice, so to me, the codeflow within this method is confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, true I'll rename it and see if I can improve more, like removing the double proxying for WFS, which does not cause errors right now thanks to ogc-client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've adapted my previous commit accordingly.

@fgravin fgravin added the bug Something isn't working label Oct 19, 2023
…sure query params encoding

otherwise first call encodes URL without query params and second call returns unmodified URL, since the host is already the window host

for more clarity also prevents double proxification for WFS, that did not fail thanks to ogc-client
@tkohr tkohr force-pushed the esri-fix-data-url branch from 75afa40 to 446a63c Compare October 19, 2023 14:04
@coveralls
Copy link

Coverage Status

coverage: 86.461% (-13.5%) from 100.0% when pulling 446a63c on esri-fix-data-url into 96f8590 on main.

Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks @tkohr

@fgravin fgravin merged commit 3c899e0 into main Oct 19, 2023
5 checks passed
@fgravin fgravin deleted the esri-fix-data-url branch October 19, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants