Skip to content

Use vector icon for nearby-map link #775

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

Merged
merged 2 commits into from
Jul 18, 2017
Merged

Conversation

akaita
Copy link
Contributor

@akaita akaita commented Jul 15, 2017

In order to optimise resource usage and render precision (same as other icons in the app), replacing this bitmap:

ic_location_on_black_24dp

With this svg:

<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 20010904//EN" "http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd">
<svg version="1.0" xmlns="http://www.w3.org/2000/svg" width="96px" height="96px" viewBox="0 0 960 960" preserveAspectRatio="xMidYMid meet">
<g id="layer1" fill="#000000" stroke="none">
 <path d="M407 801 c-187 -237 -249 -432 -181 -569 50 -101 135 -152 254 -152 119 0 204 51 254 152 54 109 28 253 -79 424 -56 90 -161 224 -175 224 -6 0 -38 -36 -73 -79z m138 -376 c19 -18 25 -35 25 -65 0 -56 -34 -90 -90 -90 -56 0 -90 34 -90 90 0 30 6 47 25 65 18 19 35 25 65 25 30 0 47 -6 65 -25z"/>
 </g>

</svg>
Drawer before the change (Nexus 5x) Drawer after the change (Nexus 5x)
drawer_with_png_mapmarker drawer_with_svg_mapmarker

@codecov-io
Copy link

codecov-io commented Jul 15, 2017

Codecov Report

Merging #775 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #775   +/-   ##
=====================================
  Coverage     6.8%   6.8%           
=====================================
  Files          94     94           
  Lines        5073   5073           
  Branches      460    460           
=====================================
  Hits          345    345           
  Misses       4700   4700           
  Partials       28     28

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0da5145...7b34717. Read the comment docs.

@tobias47n9e
Copy link
Member

Is it required for SVG graphics on Android to be stored in this kind of XML format? A real SVG would have the advantage that non-coders can easily edit the vectors. But overall I approve this change.

@psh
Copy link
Collaborator

psh commented Jul 15, 2017

Sadly, Android doesn't support SVG directly. You have to import the file using the Vector Asset Studio. The "place" icon is one of the standard material design icons from Google (see https://material.io/icons/#ic_place) so I don't think there's a need to store an SVG in the git repo.

@akaita
Copy link
Contributor Author

akaita commented Jul 15, 2017

@psh I think you are confused. Using versions of Support Library higher than 23.2 allows you to use svg in Android. In fact, the app is already using svg for some of its icons.

True, Google offers a place icon svg we could use instead of the one I put into the PR. This means it would probably be better to put that svg into the app; but the svg (even if it comes from Google) has to be stored in the repo because it is not part of Android OS.

I will replace my svg code for the one provided by Google. If you know about a way to avoid adding the svg to the repo, could you please provide an example?

@akaita akaita changed the title Use vector icons for nearby-map markers #621 Use vector icon for nearby-map link Jul 16, 2017
@misaochan misaochan merged commit f856e75 into commons-app:master Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants