#10516 added ellipsis if more than 5 warnings occurs #10595
Conversation
|
@tyrasd can you tell me what I should do next or guide me further |
There was a problem hiding this comment.
Generally, iD uses chevron icons for collapsed/collapsible UI sections. It would be good to reuse this design pattern here.
In the iD code this is called a disclosure (see modules/ui/disclosure.js).
Maybe it would be most consistent to just use a similar collapsible list of warnings in the commit section as in the validation panel:
See modules/ui/sections/validation_issues.js for how that was implemented.
What do you think?
--
PS: can you please exclude the unrelated changes in the data directory from this PR?
There was a problem hiding this comment.
Thanks! A few more things need to be adjusted from a styling perspective:
- to restore the whitespace/margins around the section: please wrap the selection in a div with css class
modal-section(see how thiscontainerwas set up in the previous implementation) - also, the yellow/red background color should be still applied for the respective warning/error sections (css class
.warning-section/.error-section). Alternatively, we could change the styling to how it looks like in the validation sidebar (colored individual warning lines, see screenshot above). I don't have a strong preference between these two options.
|
|
|
restored the css class of ul as it was before (i.e. to change-list) |
There was a problem hiding this comment.
Thanks, optically it looks fine now. There is a small regression caused by the changed css class in the last commit: the list is not duplicated when collapsing the section and un-collapsing it again. See below for a suggestion to fix the bug and at the same time improve the implementation slightly.
Additionally, I included some general code style suggestions below.
| } | ||
| } | ||
|
|
||
| function renderIssuesList(selection, severity, issues) { |
There was a problem hiding this comment.
The function argument severity does not seem to be used inside the function. Please drop it.
| } | ||
| } | ||
|
|
||
| function renderIssuesList(selection, severity, issues) { | ||
| selection.selectAll('.issues-list').remove(); |
There was a problem hiding this comment.
While this could work (if the correct css class was referenced in the d3 select 😅 ), the approach to recreate the list from scratch every time the list needs to be rendered is not the best. See the suggestion in the next comment for a solution that does actually use a d3 data binding to make sure the ul is only created if needed, and an existing ul is reused if already present.
| selection.selectAll('.issues-list').remove(); | ||
| var container = selection | ||
| .append('ul') | ||
| .attr('class', 'changeset-list'); | ||
|
|
||
| container.exit().remove(); |
There was a problem hiding this comment.
| selection.selectAll('.issues-list').remove(); | |
| var container = selection | |
| .append('ul') | |
| .attr('class', 'changeset-list'); | |
| container.exit().remove(); | |
| let container = selection | |
| .selectAll('.changeset-list') | |
| .data([0]); | |
| container = container.enter() | |
| .append('ul') | |
| .attr('class', 'changeset-list') | |
| .merge(container); |
Notice the .data([0]) here: this assigns a constant 0 as a kind of dummy data to the list: this results in the list only to only be created once (in .enter().append(…)) and only if the selection is empty.
| for (var severity in issuesBySeverity) { | ||
| var issues = issuesBySeverity[severity]; | ||
| for (let severity in _issuesBySeverity) { | ||
| let issues = _issuesBySeverity[severity]; |
There was a problem hiding this comment.
good idea to refactor some of the old code using var. Could you please also use const/let instead of var in the remaining new code of this PR?
| .disclosureContent(function(selection) { | ||
| return renderIssuesList(selection, severity, issues); | ||
| }) |
There was a problem hiding this comment.
this could be an arrow function to be more concise and consistent with the .label() property a few lines above
| .disclosureContent(function(selection) { | |
| return renderIssuesList(selection, severity, issues); | |
| }) | |
| .disclosureContent(selection => renderIssuesList(selection, ~~severity,~~ issues)) |
same also for the shouldDisplay callback and several ones called in renderIssuesList.
added ellipsis if more than 5 warnings occurs it will create an ellipsis which hold the additional warnings and we can use the final upload button without any warnings disturbances
fixes #10516