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 8 commits into
base: develop
Choose a base branch
from
Open

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

Changes from 4 commits
Commits
Show all changes
8 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
abdae29
updated forked repo
AkashNegi1 Dec 23, 2024
9dd5628
changed var to let, implemented arrow function where needed, and othe…
AkashNegi1 Dec 26, 2024
0699a21
Merge branch 'openstreetmap:develop' into develop
AkashNegi1 Jan 30, 2025
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];
tyrasd marked this conversation as resolved.
Show resolved Hide resolved

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);
})
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.

Copy link
Author

Choose a reason for hiding this comment

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

i've tried all of the above things but there's problem occuring
image
when i hit save commitWarnings() function is called twice and when click anywhere in the screen again it called and new sections of warnings (duplicate sections) are being added at the bottom
@tyrasd can you help me with this

Copy link
Author

Choose a reason for hiding this comment

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

applied all the requested changes
image
@tyrasd need to review

Copy link
Author

Choose a reason for hiding this comment

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

@tyrasd if you are not busy could you please review it

.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', 'layer-list issues-list ' + severity + 's-list');

container.exit().remove();

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;
}