-
Notifications
You must be signed in to change notification settings - Fork 36
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 kml selection exports #5300
base: production
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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