feat-deletion-in-changeset-comment dropdown #11003
Conversation
draunger
changed the title
tordans
left a comment
tordans
left a comment
There was a problem hiding this comment.
A few thoughts I had when reading the code in Github.
| osm.userChangesets(function (err, changesets) { | ||
| if (err) return; | ||
|
|
||
| var deleted = (JSON.parse(localStorage.getItem('deletedChangesetComments') || '[]')).map(s => s.trim().toLowerCase()); |
There was a problem hiding this comment.
This logic .map(s => s.trim().toLowerCase()); is duplicated in a lot of places. Do we really need it? We only ever compare the full string, to it should be OK to compare it as it. Otherwise we might want to extract it into a helper?
Another way would be to only store and compare hashes of those lines… but I don't think that is needed / has benefits.
There was a problem hiding this comment.
I would even go as far as to store the changeset id of the to be ignored comments instead of the actual literal comment string itself. That would be more memory efficient and be the most stable implementation.
There was a problem hiding this comment.
what also needs to be taken into account in the store-changeset-id approach is that in case that there exists more than one changeset with the to be ignored comment, all respective changeset ids need to be added to the list. but this does come with an implicit benefit: when the user at some later point manually enters a previously deleted comment, it will be again visible in the suggestions (until deleted again). When storing the comment itself, a user would not be able to undo an accidental deletion of a specific comment.
| d3_select(this).text(d.value); | ||
| option.text(d.value); | ||
| } | ||
| // Only for comment combobox: add cross button, floated right, minimal style |
There was a problem hiding this comment.
Can we somehow extract this into a separate file? I don't think it is ideal when this code block has to be read by every one who works on this component when it is really a separate component or feature.
There was a problem hiding this comment.
yeah… at the very least we need to make this not hardcoded to a specific UI field: The logic should be reusable for other fields like the changeset source for example.
I could think of two ways to approach this:
- leave the rendering code here, but introduce a new configuration method to enable "removable" entries for a specific combobox (
uiCombobox(…).setRemovable(callback)). The callback function is called whenever an×is clicked on. - use the existing possibility to supply the
displayproperty on the to be rendered comments , and append the×and respective logic there.
As the logic is currently only needed for the changeset editor, maybe the second approach is more efficient…
| let valueNorm = d.value.trim().toLowerCase(); | ||
| if (!deleted.map(s => s.trim().toLowerCase()).includes(valueNorm)) { | ||
| deleted.push(d.value); | ||
| localStorage.setItem('deletedChangesetComments', JSON.stringify(deleted)); |
There was a problem hiding this comment.
How will this get reset? Like, when I click it 100 times, all those will be in the stack forever, right? Should we maybe allow for a max of 100 items and remove the last items? Like a max array size…
There was a problem hiding this comment.
It should be safe to remove any stored entries that do not match any changeset that is currently returned by the OSM API anymore.
| if (klass === 'comment') { | ||
| option.append('button') | ||
| .attr('class', 'combobox-delete') | ||
| .attr('title', 'Delete this comment') |
There was a problem hiding this comment.
User facing texts should never be hardcoded to English. See https://github.com/openstreetmap/iD/wiki/How-to-Use-iD%27s-Localizer-Module for how to add and use translatable strings.
|
closing as stale |
3 participants
description :
fixes :
#10957