Remarks #18
Remarks #18
Conversation
www/static/audit.js
Outdated
| wereCoord = props['were_coords']; | ||
|
|
||
| wereCoord = props['were_coords'], | ||
| remarks = pop(props, 'remarks'); |
There was a problem hiding this comment.
I think this wasn't clear to me, but this is not necessary at all. I'll update.
www/static/audit.js
Outdated
| @@ -435,6 +447,11 @@ function displayPoint(data, audit) { | |||
| } | |||
| } | |||
|
|
|||
| // render remarks, if any. | |||
| if (remarks) { | |||
| document.getElementById("remarks_box").innerHTML = "<h3>Remarks</h3><p>" + remarks + "</p>"; | |||
There was a problem hiding this comment.
The shorter added segment is, the better. Also, this does not screen HTML in remarks. I'd suggest adding <b>Remarks:</b> <span id="remarks_content"></span> in the div, add $('#remarks_box').show(); $('#remarks_content').text(remarks) here, and $('#remarks_box').hide() below, when other UI elements are hidden.
There was a problem hiding this comment.
I'll update this as well, thanks for your review and inputs!
www/static/audit.js
Outdated
|
|
||
| wereCoord = props['were_coords'], | ||
| remarks = props['remarks']; | ||
|
|
There was a problem hiding this comment.
I think you've added four spaces in this empty line. Could you remove them please?
www/static/audit.js
Outdated
| $('#remarks_content').text(remarks); | ||
| } else { | ||
| $('#remarks_box').hide(); | ||
| $('#remarks_content').text(""); |
There was a problem hiding this comment.
You don't have to clear this <span>, since it is not visible.
Regarding the .hide(), please add it also to hidePoint() function below, and somewhere here.
|
Is this OK now? Let me know if I can ameliorate anything. |
www/static/audit.js
Outdated
| if (remarks) { | ||
| $('#remarks_box').show(); | ||
| $('#remarks_content').text(remarks); | ||
| } |
There was a problem hiding this comment.
hidePoint() is not called when one feature replaces another, so you still have to add that else block here.
There was a problem hiding this comment.
Isn't that already taken care of by the line here? (default hidden, only shown if remarks are truthy): https://github.com/mapsme/cf_audit/pull/18/files#diff-c8d8df3d3735fa40f4df0b92c1776962R94
There was a problem hiding this comment.
Just for the first object: that is the initialization code. After that, many displayPoint() are called in succession.
There was a problem hiding this comment.
Now I understand the code a bit better, thank you :-)
|
I am at my day job currently, so don't be hasty, I can disappear for few hours :) |
|
Didn't wanted to appear hasty, sorry to have given that impression ;-) |
|
Thanks, looks good now! |
This pull-request only makes sense if this one is accepted as well: mapsme/osm_conflate#13
Cleanup, which isn't directly related to the base functionality: