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.

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

Write browse element icons as <img> #5385

Merged

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Dec 6, 2024

A smaller version of #5355 that doesn't include way node folding and grouping. Things should look mostly the same with better icon alignment. Although as you can see below, the priorities of icons if several are applicable are different. If an element has both shop and building tags, which icon should we show?

Before / after:
image / image

Before / after:
image / image


There are three different PRs for browse element lists:

@github-actions github-actions bot added the big-pr label Dec 6, 2024
Copy link

github-actions bot commented Dec 6, 2024

1 Warning
⚠️ Number of updated lines of code is too large to be in one PR. Perhaps it should be separated into two or more?

Generated by 🚫 Danger

@AntonKhorev AntonKhorev force-pushed the browse-element-icons-inline branch from c8be6c0 to df6dd0a Compare December 6, 2024 15:18
@AntonKhorev AntonKhorev marked this pull request as ready for review December 6, 2024 15:37
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

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.

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.

My principle suggestion is that a key/value rule should always win over a key only rule as it's more specific.

If we want total backwards compatibility then the first matching rule should win - an way to do that would be to walk target_tags in reverse order when looking for a match.

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.

app/helpers/browse_helper.rb Outdated Show resolved Hide resolved
<%= element_single_current_link "node", wn.node %>
<% related_ways = wn.node.ways.reject { |w| w.id == wn.way_id } %>
<% icon_connector = " " %>
Copy link
Member

Choose a reason for hiding this comment

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

Why use a variable for this when it doesn't ever seem to change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was the easiest way to stop linters from complaining. Otherwise they tell to use interpolation. But as soon as I use interpolation, html gets escaped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And it won't exist if we do #5317. Not having to deal with inline icons is one of the goals of that PR.

@AntonKhorev AntonKhorev force-pushed the browse-element-icons-inline branch from df6dd0a to dc52cdc Compare December 9, 2024 10:26
@AntonKhorev AntonKhorev requested a review from tomhughes December 9, 2024 10:38
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

Hmm that style warning is a bit annoying, because string interpolation would be really nasty there, so I guess we live with the dummy variable for now.

Other than that is looks good now.

@tomhughes tomhughes merged commit 59fccd9 into openstreetmap:master Dec 10, 2024
22 checks passed
@AntonKhorev AntonKhorev deleted the browse-element-icons-inline branch December 11, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants