Skip to content
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.

Sign up for GitHub

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

#10516 added ellipsis if more than 5 warnings occurs #10595

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

#10516 added ellipsis if more than 5 warnings occurs #10595

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c598143
#10516 added ellipsis if more than 5 warnings occurs
AkashNegi1 Dec 11, 2024
03e4d02
added chevron icon UI to commitWarning
AkashNegi1 Dec 12, 2024
7c5b0bb
removed unrelated changes from data directory
AkashNegi1 Dec 12, 2024
d11014d
fixed CSS of the warning section
AkashNegi1 Dec 14, 2024
6b2ee66
restored the css class of the <ul> to changeset-list
AkashNegi1 Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 88 additions & 78 deletions modules/ui/commit_warnings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,103 +1,113 @@
import { select as d3_select } from 'd3-selection';

import { t } from '../core/localizer';
import { svgIcon } from '../svg/icon';
import { uiTooltip } from './tooltip';
import { uiSection } from '../ui/section';
import { utilEntityOrMemberSelector } from '../util';


export function uiCommitWarnings(context) {
var _issuesBySeverity = {};

function commitWarnings(selection) {
var issuesBySeverity = context.validator()
// Wrap the selection in a div with the class modal-section
selection = selection.append('div').attr('class', 'modal-section');

// Load issues by severity
_issuesBySeverity = context.validator()
.getIssuesBySeverity({ what: 'edited', where: 'all', includeDisabledRules: true });

for (var severity in issuesBySeverity) {
var issues = issuesBySeverity[severity];
for (let severity in _issuesBySeverity) {
let issues = _issuesBySeverity[severity];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if (severity !== 'error') { // exclude 'fixme' and similar - #8603
if (severity !== 'error') { // exclude 'fixme' and similar - #8603
issues = issues.filter(function(issue) { return issue.type !== 'help_request'; });
}

var section = severity + '-section';
var issueItem = severity + '-item';

var container = selection.selectAll('.' + section)
.data(issues.length ? [0] : []);

container.exit()
.remove();

var containerEnter = container.enter()
.append('div')
.attr('class', 'modal-section ' + section + ' fillL2');

containerEnter
.append('h3')
.call(severity === 'warning' ? t.append('commit.warnings') : t.append('commit.errors'));

containerEnter
.append('ul')
.attr('class', 'changeset-list');

container = containerEnter
.merge(container);

if (!issues.length) continue;

var items = container.select('ul').selectAll('li')
.data(issues, function(d) { return d.key; });

items.exit()
.remove();

var itemsEnter = items.enter()
.append('li')
.attr('class', issueItem);

var buttons = itemsEnter
.append('button')
.on('mouseover', function(d3_event, d) {
if (d.entityIds) {
context.surface().selectAll(
utilEntityOrMemberSelector(
d.entityIds,
context.graph()
)
).classed('hover', true);
}
// Create a collapsible section for each severity level
var section = uiSection('issues-' + severity, context)
.label(() => {
var count = issues.length;
return t.append(
'inspector.title_count',
{ title: t('issues.' + severity + 's.list_title'), count: count }
);
})
.on('mouseout', function() {
context.surface().selectAll('.hover')
.classed('hover', false);
.disclosureContent(function(selection) {
return renderIssuesList(selection, severity, issues);
})
Comment on lines +36 to 38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be an arrow function to be more concise and consistent with the .label() property a few lines above

Suggested change
.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.

.on('click', function(d3_event, d) {
context.validator().focusIssue(d);
.shouldDisplay(function() {
return issues && issues.length;
});

buttons
.call(svgIcon('#iD-icon-alert', 'pre-text'));

buttons
.append('strong')
.attr('class', 'issue-message');

buttons.filter(function(d) { return d.tooltip; })
.call(uiTooltip()
.title(function(d) { return d.tooltip; })
.placement('top')
);

items = itemsEnter
.merge(items);

items.selectAll('.issue-message')
.text('')
.each(function(d) {
return d.message(context)(d3_select(this));
});
// Add the appropriate class for styling based on severity
selection
.call(section.render)
.classed(severity + '-section', true);
}
}

function renderIssuesList(selection, severity, issues) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function argument severity does not seem to be used inside the function. Please drop it.

selection.selectAll('.issues-list').remove();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var container = selection
.append('ul')
.attr('class', 'changeset-list');

container.exit().remove();
Comment on lines +51 to +56
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.


var items = container.selectAll('li')
.data(issues, function(d) { return d.key; });

items.exit().remove();

var itemsEnter = items.enter()
.append('li')
.attr('class', function (d) { return 'issue severity-' + d.severity; });

var buttons = itemsEnter
.append('button')
.on('mouseover', function(d3_event, d) {
if (d.entityIds) {
context.surface().selectAll(
utilEntityOrMemberSelector(
d.entityIds,
context.graph()
)
).classed('hover', true);
}
})
.on('mouseout', function() {
context.surface().selectAll('.hover').classed('hover', false);
})
.on('click', function(d3_event, d) {
context.validator().focusIssue(d);
});

var textEnter = buttons
.append('span')
.attr('class', 'issue-text');

textEnter
.append('span')
.attr('class', 'issue-icon')
.each(function(d) {
var iconName = '#iD-icon-' + (d.severity === 'warning' ? 'alert' : 'error');
d3_select(this)
.call(svgIcon(iconName));
});

textEnter
.append('span')
.attr('class', 'issue-message');

itemsEnter
.merge(items)
.selectAll('.issue-message')
.text('')
.each(function(d) {
return d.message(context)(d3_select(this));
});
}

return commitWarnings;
}