Feature/1099 infoclick parse xml #1256
Conversation
Parse XML data and use the attributes of the FIELDS element as feature properties.
Parse XML and return an array of features.
In cases where the xml have no FIELDS element, the xml is parsed as gml.
|
Ready for review and merge @linusfj? In that case, please request a reviewer in the side panel. |
|
Yes, it is ready for review, and I've requested reviewers. |
jacobwod
added
the
needs testing in rc
There was a problem hiding this comment.
A couple of questions follow.
Why do we need a custom XML parser here? Wouldn't the way shown in OL example be sufficient enough? Is the response malformed in some way?
new-client/src/models/Click.js
Outdated
| case "text/xml": { | ||
| featurePromises.push( | ||
| response.value.requestResponse | ||
| .text() | ||
| .then((text) => { | ||
| if (text !== undefined && text) { | ||
| features.push( | ||
| ...getFeaturesFromXml(response.value, text) | ||
| ); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| console.error( | ||
| "GetFeatureInfo couldn't retrieve correct data for the clicked object.", | ||
| err | ||
| ); | ||
| }) | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Also, this part looks very much like the next that follows, the only difference being which parser is used depending on XML or GML, i.e. lines 314 and 332. The rest is just the same so consider refactoring.
|
The response from the WMS example in issue #1099 was not displayed correctly in the infoclick dialog. It appears that this issue is specific to WMS layers that use ArcGIS server, and it is still present in the new version of the infoclick function. |
- Sorting of features does not mutate the array. Insteand it returns a new array. - const is used where appropiate - Minor esthetic changes.
|
Check it out now and see if it works as intended for you. |
|
Please confirm when you've tested the latest commits @linusfj. |
|
It seems to not be working, when clicking on a feature an error message appears: |
|
OK, take a look at what it is and try to fix it. I could not reproduce the error you mention on my side. |
Fix bug in multiple functions where returned arrays were not sorted in place. Updated code to sort original arrays and return them with expected values.
|
The issue has been fixed and the code can be merged. |
|
OK, good find. Since we're now not using the return value form If you want to make it even nicer, you can rename |
- Changed variables 'parsed' to 'features'. - Changed function name 'sortFeatures' to 'sortAndMutateFeaturesArray'. - Used `.map()` function to add the `layer` property to each feature. - Removed return statements. - Modified the array `features` to be defined as `let` instead of `const`, since it can be reassigned with new values.
|
Btw, this is not part of this issue but I'll post it here: as I reviewed the code, it seems to look like the sorting algorithm only takes place in As I said, not part of this PR (it's ready to merge in my opinion). But I wonder what you think of it? |
|
I've only tested the new infoclick version using a few layers with different data formats that we initially had problems with, and it was returning the correct results in those cases. The new implementation in |
|
Merging pull request. |
Closes #1099.