Conversation
| .call(drawListItems, ['highlight_edits'], 'checkbox', 'visual_diff', toggleHighlightEdited, function() { | ||
| return context.surface().classed('highlight-edited'); | ||
| }); | ||
| .call(drawListItems, ['highlight_edits'], 'checkbox', 'visual_diff', toggleHighlightEdited, () => |
There was a problem hiding this comment.
It would be great to put this into the coding style. Do we use arrow functions or not; and if, when… – Or is it OK, when we mix it up randomly…
In all my other projects I am all in on arrow functions inline and for helper. But for iD I tried to always use the other pattern to keep it the same…
There was a problem hiding this comment.
+1 for recommending arrow functions for anonymous inline functions. Specifically with d3 there are a few cases where regular functions are still needed, e.g. to access this in the callback of .each(function(d) { d3_select(this).… }.
There was a problem hiding this comment.
put this into the coding style
This is actually already covered by the linked code style guidelines for Javascript, e.g. https://github.com/airbnb/javascript?tab=readme-ov-file#arrow-functions and similarly for the recommendation for const/let instead of var: https://github.com/airbnb/javascript?tab=readme-ov-file#references
modules/globals.d.ts
Outdated
| declare var VITEST: true; | ||
|
|
||
| declare type Tags = { [key: string]: string }; | ||
| declare type Tags = { [key: string]: string | undefined }; |
There was a problem hiding this comment.
also here: //cc @k-yle : I'm not sure this is superbly elegant, but a lot of front end code uses a tags object where setting a particular entry's value to undefined is interpreted as this tag should be deleted.
There was a problem hiding this comment.
it's tricky because this will make it less convenient for other files where undefined is never used as a value. maybe we need 3 different interfaces for the 3 different cases?
{ [key: string]: string }
{ [key: string]: string | undefined }
{ [key: string]: string | string[] }e.g. check.js uses the 3rd case right? arrays are used when there's a multi-selection with different values
There was a problem hiding this comment.
Good point, for multi-selections the value can be a string[] too1. E.g. as checked in Array.isArray(_tags[field.key].
But check.ts also uses the explicit undefined version also, see line 48: values = [undefined, 'yes'];, later this is assigned to the updated tags in t[field.key] = values[…];.
But I think the two/three cases are luckily mutually exclusive: undefined as a value is only allowed when updating the tags of objects, and string[] is only occurring as the input of the ui fields. I committed 70d97e5 with a potential way to divide this up.
Footnotes
-
PS: Honestly, I would consider the way multiselections are encoded as these
key=<string | string[]>construct as very suboptimal if not to say hacky. This makes handling them correctly quite cumbersome in places where the tags are not trivially transformable to fields (e.g. here). Maybe after we have this all as typescript, this can be refactored with confidence and without too much pain toTags[]. ↩
There was a problem hiding this comment.
💯 yeah definitely, i think 70d97e5 is a good solution
* `Tags` - simple/regular tags of a single entity * `TagsMulti` - tags of a multi-selection: can be either normal key/value tags or value as an array when there are conflicting tags present * `TagsUpdate` - for the results of an operation, the values here can be set to `undefined` to indicate that the respective tag should be deleted from the respective entities
3 participants
In order to avoid oversights such as 0241c76 from slipping through (incomplete) unit tests as well as manual testing, it is probably necessary to port the frontend modules over to typescript.
As a proof of concept here is how this might look like for one of the UI code:
modules/ui/sections/map_style_options.jsmodules/ui/fields/check.tsmodules/ui/cmd_sequence.tsin refactoruiCmdSequenceinto a separate function #11998Notably, for
d3.selectone needs to include the respective type hints, e.g.selection.selectAll<HTMLUListElement, any>(ul.…), and interfacing to existing not-type annotated modules sometimes require an explicit case toany(e.g. inuiSectionMapStyleOptions(context: any), or(uiTooltip() as any).…), at least as long as we don't port them over as well.Feedback welcome!