From 771ad4dbd91c966d4526569bbde1def423c63efe Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Mon, 10 Oct 2022 13:28:00 +0200 Subject: [PATCH] fix calculation of access field placeholder on multiselections --- CHANGELOG.md | 1 + modules/ui/fields/access.js | 75 +++++++++++++++++++---------------- modules/util/util.js | 3 ++ test/spec/ui/fields/access.js | 24 +++++++++++ 4 files changed, 68 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f87cc5d8b..f1d47f0a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ _Breaking developer changes, which may affect downstream projects or sites that * Better handling of rate limited API calls and other API errors ([#10299]) * Fix info boxes for descriptions of brands referenced by [NSI](https://github.com/osmlab/name-suggestion-index) presets ([#10885]) * Make features clickable when "Full Fill" rendering style is selected +* Fix calculation of access field placeholders for multi selections ([#9333]) #### :earth_asia: Localization #### :hourglass: Performance #### :mortar_board: Walkthrough / Help diff --git a/modules/ui/fields/access.js b/modules/ui/fields/access.js index 68589c470..0693252af 100644 --- a/modules/ui/fields/access.js +++ b/modules/ui/fields/access.js @@ -277,55 +277,60 @@ export function uiFieldAccess(field, context) { utilGetSetValue(items.selectAll('.preset-input-access'), function(d) { return typeof tags[d] === 'string' ? tags[d] : ''; }) - .classed('mixed', function(d) { - return tags[d] && Array.isArray(tags[d]); + .classed('mixed', function(accessField) { + return tags[accessField] && Array.isArray(tags[accessField]) + || new Set(getAllPlaceholders(tags, accessField)).size > 1; }) - .attr('title', function(d) { - return tags[d] && Array.isArray(tags[d]) && tags[d].filter(Boolean).join('\n'); + .attr('title', function(accessField) { + return tags[accessField] && Array.isArray(tags[accessField]) && tags[accessField].filter(Boolean).join('\n'); }) - .attr('placeholder', function(d) { - if (tags[d] && Array.isArray(tags[d])) { + .attr('placeholder', function(accessField) { + let placeholders = getAllPlaceholders(tags, accessField); + if (new Set(placeholders).size === 1) { + // all objects have the same implied access + return placeholders[0]; + } else { return t('inspector.multiple_values'); } - if (d === 'bicycle' || d === 'motor_vehicle') { - if (tags.vehicle && typeof tags.vehicle === 'string') { - return tags.vehicle; - } - } - if (tags.access && typeof tags.access === 'string') { - return tags.access; - } - function getPlaceholdersByTag(key, placeholdersByKey) { - if (typeof tags[key] === 'string') { - if (placeholdersByKey[tags[key]] && - placeholdersByKey[tags[key]][d]) { - return placeholdersByKey[tags[key]][d]; - } - } else { - var impliedAccesses = tags[key].filter(Boolean).map(function(val) { - return placeholdersByKey[val] && placeholdersByKey[val][d]; - }).filter(Boolean); + }); - if (impliedAccesses.length === tags[key].length && - new Set(impliedAccesses).size === 1) { - // if all the barrier values have the same implied access for this type then use that - return impliedAccesses[0]; - } - } + function getAllPlaceholders(tags, accessField) { + let allTags = tags[Symbol.for('allTags')]; + if (allTags && allTags.length > 1) { + // multi selection + const placeholders = []; + allTags.forEach(tags => { + placeholders.push(getPlaceholder(tags, accessField)); + }); + return placeholders; + } else { + return [getPlaceholder(tags, accessField)]; + } + } + + function getPlaceholder(tags, accessField) { + if (tags[accessField]) { + return tags[accessField]; + } + if (tags.vehicle && (accessField === 'bicycle' || accessField === 'motor_vehicle')) { + return tags.vehicle; + } + if (tags.access) { + return tags.access; } for (const key in placeholdersByTag) { if (tags[key]) { - const impliedAccess = getPlaceholdersByTag(key, placeholdersByTag[key]); - if (impliedAccess) { - return impliedAccess; + if (placeholdersByTag[key][tags[key]] && + placeholdersByTag[key][tags[key]][accessField]) { + return placeholdersByTag[key][tags[key]][accessField]; } } } - if (d === 'access' && !tags.barrier) { + if (accessField === 'access' && !tags.barrier) { return 'yes'; } return field.placeholder(); - }); + } }; diff --git a/modules/util/util.js b/modules/util/util.js index 68dc61d50..58a223385 100644 --- a/modules/util/util.js +++ b/modules/util/util.js @@ -298,6 +298,7 @@ export function utilCombinedTags(entityIDs, graph) { var tags = {}; var tagCounts = {}; var allKeys = new Set(); + var allTags = []; var entities = entityIDs.map(function(entityID) { return graph.hasEntity(entityID); @@ -312,6 +313,7 @@ export function utilCombinedTags(entityIDs, graph) { }); entities.forEach(function(entity) { + allTags.push(entity.tags); allKeys.forEach(function(key) { @@ -358,6 +360,7 @@ export function utilCombinedTags(entityIDs, graph) { }); } + tags = Object.defineProperty(tags, Symbol.for('allTags'), { enumerable: false, value: allTags }); return tags; } diff --git a/test/spec/ui/fields/access.js b/test/spec/ui/fields/access.js index cab0f527d..af7d13fbd 100644 --- a/test/spec/ui/fields/access.js +++ b/test/spec/ui/fields/access.js @@ -112,4 +112,28 @@ describe('iD.uiFieldAccess', function() { expect(selection.selectAll('.preset-input-access-motor_vehicle').attr('placeholder')).to.equal('destination'); }); + it('sets bicycle and motor_vehicle placeholder to the value of the "vehicle" tag (id-tagging-schema#378)', function() { + var access = iD.uiFieldAccess(field, context); + selection.call(access); + + access.tags({highway: 'residential', vehicle: 'destination'}); + expect(selection.selectAll('.preset-input-access-motor_vehicle').attr('placeholder')).to.equal('destination'); + expect(selection.selectAll('.preset-input-access-bicycle').attr('placeholder')).to.equal('destination'); + }); + + it('sets correct placeholder on a multi selection', function() { + var access = iD.uiFieldAccess(field, context); + selection.call(access); + + var tags = {highway: 'primary', foot: ['yes', 'no'], bicycle: ['no', undefined], vehicle: ['no', undefined]}; + tags[Symbol.for('allTags')] = [ + {highway: 'primary', foot: 'yes', bicycle: 'no'}, + {highway: 'primary', foot: 'no', vehicle: 'no'} + ]; + access.tags(tags); + expect(selection.selectAll('.preset-input-access-foot').attr('placeholder')).to.equal(iD.localizer.t('inspector.multiple_values')); + expect(selection.selectAll('.preset-input-access-bicycle').attr('placeholder')).to.equal('no'); + expect(selection.selectAll('.preset-input-access-motor_vehicle').attr('placeholder')).to.equal(iD.localizer.t('inspector.multiple_values')); + }); + });