Add kml selection exports #5300
Conversation
Fix type errors (TODO: use XmlNode)
Fix data format Add xml header Attempt to fix types
|
While I'm sure that this is done to mimic CSV on frontend, I think a better implementation would be just to make an API call, but this time adding an extra filter to just select the ids that the user chose. Benefits:
@maxpatiiuk thoughts? I think, even for CSV, it would have been better to directly use backend. |
There was a problem hiding this comment.
Good use of the utility functions that we have available!
Nevertheless, I agree with Vinny that if it would reduce duplication, we should do this work on the back-end.
Aside from that, better communication about who is working on what is always nice to avoid redundant effort
| ); | ||
|
|
||
|
|
||
| let placemarkTarget: any = []; |
There was a problem hiding this comment.
reduce usages of any.
in this case the type XmlNode might be better
| // <ExtendedData> | ||
| let extendedDataTarget: any = []; | ||
|
|
||
| fields.forEach((field, index) => { |
There was a problem hiding this comment.
this could be implemented using .map() too
see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map
that would match the code style in the rest of the codebase and reduce the need for explicitly specifying typescript types
| @@ -50,7 +50,7 @@ export const xmlToJson = (element: Element): XmlNode => ({ | |||
| /** | |||
| * Reverse conversion to JSON | |||
| */ | |||
| export function jsonToXml(node: XmlNode): Element { | |||
| export function jsonToXml(node: any): Element { | |||
There was a problem hiding this comment.
please refrain from using any in our codebase.
any disabled type-checking, which negates the benefits of TypeScript
you can read a quick reference guide on TypeScript if you wish to be more familiar with TypeScript concepts: https://www.typescriptlang.org/docs/handbook/typescript-in-5-minutes.html
you can also use chatgpt to explain TypeScript errors
if you are using VS Code, you can also install this extension to make the error messages more readable: https://marketplace.visualstudio.com/items?itemName=yoavbls.pretty-ts-errors
There was a problem hiding this comment.
Aha, I didn't think my code would be reviewed yet 😅 I promise this was a temporary hack as I was still figuring out the xml types.
There was a problem hiding this comment.
Ah, that's my mistake, I didn't mean to trigger a code review. Nevertheless, these resources by Max are good starting points!
I agree. I already came across issues myself having to implement the same feature twice on my last CSV export PR. I think the only downside is we'd lose the instant downloads from the frontend exports. |
|
I don't think it necessarily matters, especially for KML exports. For CSV, it is quite handy to do it within a few clicks, so don't change that (or users wouldn't like extra clicks needed now). For KML, definitely go the backend route. You'd learn about the query builder API quite a bit in the process! |
Fixes #3589
Adds frontend implementation of KML exports using jsonToXml
WIP
Checklist
and self-explanatory (or properly documented)
Testing instructions