<p></p>
<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p dir="auto">So to summarise on the question of what happens when there are multiple icons the old logic is that priority is defined by the order of the rules in the CSS which is mostly alphabetical except that the generic building rule is with the way rules which come after the node rules although the node/way split is really nonsense.</p>
<p dir="auto">The new logic handles everything alphabetically, except that now the last rule wins rather than the first, and building is now ordered with other things so loses to shop but beats amenity.</p>
<p dir="auto">My principle suggestion is that a key/value rule should always win over a key only rule as it's more specific.</p>
<p dir="auto">If we want total backwards compatibility then the first matching rule should win - an way to do that would be to walk <code class="notranslate">target_tags</code> in reverse order when looking for a match.</p>
<p dir="auto">I'm not sure that really matters though - if there are multiple key/value matches then there's no real right or wrong answer as to which one to choose.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/5385#discussion_r1874750180">app/helpers/browse_helper.rb</a>:</p>
<pre style='color:#555'>> @@ -1,15 +1,43 @@
module BrowseHelper
+ def element_icon(type, object)
+ icon_data = { :filename => "#{type}.svg" }
+
+ unless object.redacted?
+ target_tags = object.tags.find_all { |k, _v| BROWSE_ICONS.key? k.to_sym }.sort
+ title = target_tags.map { |k, v| "#{k}=#{v}" }.to_sentence unless target_tags.empty?
+
+ target_tags.each do |k, v|
+ k = k.to_sym
+ v = v.to_sym
+ if v != :* && BROWSE_ICONS[k].key?(v)
+ icon_data = BROWSE_ICONS[k][v]
+ elsif BROWSE_ICONS[k].key?(:*)
</pre>
<p dir="auto">I'd suggest not allowing a wildcard key only match to override a more specific key=value match here:</p>
⬇️ Suggested change
<pre style="color: #555">- elsif BROWSE_ICONS[k].key?(:*)
+ elsif icon_data.nil? && BROWSE_ICONS[k].key?(:*)
</pre>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/5385#discussion_r1874750678">app/views/browse/_way.html.erb</a>:</p>
<pre style='color:#555'>> <%= element_single_current_link "node", wn.node %>
<% related_ways = wn.node.ways.reject { |w| w.id == wn.way_id } %>
+ <% icon_connector = " " %>
</pre>
<p dir="auto">Why use a variable for this when it doesn't ever seem to change?</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/5385#pullrequestreview-2486890217">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLPQOOMEKXH7DWUGHWT2EQTALAVCNFSM6AAAAABTE2UVTKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWHA4TAMRRG4">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLIXD34VHBZ2EJ543ST2EQTALA5CNFSM6AAAAABTE2UVTKWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUUHLXOS.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/5385/review/2486890217</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/5385#pullrequestreview-2486890217",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/5385#pullrequestreview-2486890217",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>