Browse icons as svg #5080
Browse icons as svg #5080
Conversation
Should they? How about landuses, for example? |
|
I believe the current icons originate from osm-carto, and are mostly 9-10 years old. In the meantime, carto has moved from png to svg, and updated some of their icons. In a way, this PR is catching up with changes in the default style. Landuse is a good question. I would do them in a second step, once the node icons have been migrated to svg. We don't have to do everything at once. |
Why not inline |
mmd-osm
force-pushed
the
patch/browsesvg
branch
from
eeab529
to
e3db323
Compare
|
This seems like a good idea. Would you use some Gem for it, like https://github.com/jamesmartin/inline_svg and change the browse view / browse_helper.rb accordingly? Do we already have something similar in the code base today, which we could reuse? |
mmd-osm
force-pushed
the
patch/browsesvg
branch
4 times, most recently
from
83483af
to
983f066
Compare
|
@AntonKhorev : Just for clarification: is this the approach you had in mind? https://stackoverflow.com/a/68569598/3501889 It seems the target svg need to include a "fragment identifier" to make this work, which is currently not the case for the carto svg files. See https://stackoverflow.com/a/72140723/3501889 I tried these changes in a browser window by adding the svg to the DOM, and it works fine. Changing colors is also much easier now. By the way, the inline_svg gem is loading the file when rendering the page and injecting the contents into the HTML document, which is not great for caching. |
mmd-osm
force-pushed
the
patch/browsesvg
branch
4 times, most recently
from
b6cea61
to
61867fc
Compare
|
I wanted inline svg with a bunch of openstreetmap-website/app/views/directions/search.html.erb Lines 3 to 78 in 59571f8 |
mmd-osm
force-pushed
the
patch/browsesvg
branch
from
61867fc
to
37984a5
Compare
|
Thank you for the details the inline approach! I think it would be a bit challenging to always include all symbol svg files in the browse page due to their total size (around 200K). I thought about adding some configuration table to move most of the logic from browse.scss to a yml file (at least for svg). For this purpose I have introduced a new config/browse_image.yml, and the corresponding initializer in config/initializers/browse_image.rb. (see most recent push). In some cases the same image is used for different tags, so we would need some sort of configuration anyway to map tags to svg images and define how they should be filled. In the next step BROWSE_IMAGE can then be used in helpers/browse_helper.rb to find out if an SVG exists for a "{key}_{value}", and then use the external_svg approach I mentioned in my previous post. In browse_helper.rb something similar to the following would be needed. Also, we would need to define some css classes for the new svg images. elem_icon = icon_tags(object).map { |k, v| "#{k}_#{v}" }.last
elem_icon_detail = BROWSE_IMAGE[elem_icon.to_sym]
external_svg(elem_icon_detail[:image], elem_icon_detail[:fill]) if elem_icon_detail def external_svg(identifier, fill, attributes = {})
file_name, fragment = identifier.split("#")
fragment ||= "icon"
attributes[:xmlns] = "http://www.w3.org/2000/svg"
attributes[:class] = fill
file_name = "browse/#{file_name}"
content_tag :svg, attributes do
tag.use :href => "#{image_path(file_name)}##{fragment}"
end
end |
mmd-osm
force-pushed
the
patch/browsesvg
branch
7 times, most recently
from
d772ead
to
0fcdb3d
Compare
|
I believe the code should be good for another review. The screenshot below shows mixed svg/png rendering (1 png, rest is svg). The exact symbol location probably need some further tweaking in CSS. Also, the code in browser_helper.rb might need some improvements. Focus right now should be that we're ok with the approach.
|
mmd-osm
force-pushed
the
patch/browsesvg
branch
2 times, most recently
from
96ae532
to
eb714c5
Compare
If they specify path coordinates to a millionth fraction of a pixel, it's not surprising. |
mmd-osm
force-pushed
the
patch/browsesvg
branch
from
eb714c5
to
e48f5f1
Compare
mmd-osm
force-pushed
the
patch/browsesvg
branch
from
875f22b
to
40c07ec
Compare
|
@AntonKhorev : I think I'm done now with my changes. Anything else we need to check before merging? |
|
Some icons before/after in dark mode:
|
| margin-left: -25px; | ||
| width: 25px; | ||
| height: 18px; | ||
| /*rtl:ignore*/ transform: translate(2px, 0px); |
There was a problem hiding this comment.
What's this translate for?
Old css background image above, new svg image below:
There was a problem hiding this comment.
The purpose of the translate is to improve the positioning of SVGs. Like I've written below, that's good for the majority of new SVGs.
The remaining ~20 old PNG symbols will be replaced by SVGs in later PRs, by which time this PNG/SVG positioning issue will become obsolete. The transition to SVGs clearly has the higher priority for me.
If someone feels like working on it, some fine tuning could still be done in follow up PRs.
There was a problem hiding this comment.
highway=turning_circle has been replaced by an SVG symbol.
|
I still think it's easier to switch to |
|
Thank you for reviewing this PR another time.
Agree, that's also something I've noticed. I'm pushing another commit now which should improve the situation. As a general remark, I would leave that sort of fine tuning to a follow up pull request.
Yes, it matches exactly the size you see on the map.
I've checked all 219 SVGs, and found that less than 10 of them might need some fine tuning. The vast majority of new SVGs is just fine. Hence, I think that's a good topic for follow up pull requests as well.
I'm not entirely clear what you're suggesting here. Assuming we need to convert SVGs to PNGs and maintain new entries in browse.scss for them, that would be a very clear no-go for me. Speaking of experience in this PR, it's creating a huge effort with very limited options for later reuse. I would suggest to focus on any remaining important issues with SVGs instead in this PR, rather than spending time on PNGs which we want to phase out anyway. |
No, we don't need it to match exactly the size you see on the (one particular rendering of the) map. We need to decide what size the icons should be and make them that size. |
Why making a pull request with 219 svgs with the vast majority just fine instead of a smaller pull request with all svg fine? |
I'm not suggesting that. |
mmd-osm
force-pushed
the
patch/browsesvg
branch
from
ea30534
to
783a05b
Compare
|
I've added dedicated css formatting for 9 SVG symbols now, to improve positioning and/or size of the respective symbols. Maybe you could take another look now. If necessary, please suggest exactly which symbol should be changed and how. The approach should be flexible enough to allow for further fine-tuning in the future. |
Reformatted version of https://wiki.openstreetmap.org/wiki/File:Highway_mini_roundabout.svg without the white background
Ok, I think you would need to further elaborate on your proposal, because it’s still not clear to me how this looks in detail. Please provide some working code snippets which addresses both position and coloring of svg. Also some comment why it is better or what issues it solves. We can then look into this in more detail and work out the best approach. |
The browse icons haven't been updated for quite some time and are still served as PNG icons. As the icons should reflect the current carto style, I copied a fair amount to this PR, and added some coloring similar to amenity-points.mss.
One of the design goals for the conversion was to use osm-carto svg files as-is, and at the same time use svg files as < img >, rather than inlining them. This has some implications on coloring: color values need to be translated to a filter, for which i used https://blog.union.io/code/2017/08/10/img-svg-fill/ + https://stackoverflow.com/questions/24933430/img-src-svg-changing-the-styles-with-css/52041765#52041765
Before converting more icons in browse.scss + common.scss, I wanted to get some feedback if the overall approach is sound. As an example, for the fill parameter I tried "#666666" as parameter value, as well as the original color descriptions from osm-cargo (e.g. "amenity-brown"). I find the latter a bit easier to understand.
Please take a look, and let me know how we should proceed.