Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[RFC] Add react-native-svg #1273

Closed
2 tasks done
l2succes opened this issue Jan 28, 2019 · 11 comments
Closed
2 tasks done

[RFC] Add react-native-svg #1273

l2succes opened this issue Jan 28, 2019 · 11 comments
Assignees
Labels

Comments

@l2succes
Copy link
Contributor

l2succes commented Jan 28, 2019

New Dependency

Name: react-native-svg
URL: https://github.com/react-native-community/react-native-svg

Focus

Enables us to render scalable icons and dynamic change styling size based on current state on our map view and anywhere in the app. In the case of local discovery we have about 4 different type of icons with 3 states each. Moreover, all our icons in palette are already SVGs (https://github.com/artsy/palette/tree/master/src/svgs) so this library would allow us to use them in emission as well and further our goal of using palette as the source of truth for our style system.

Check List

  • Have read over the source code, and we can maintain it
  • Has had a release in the last year, or looks done and stable

Alternatives

  • Using PNGs
    We already tried using PNGs instead and pins would look pixelated at higher zoom levels when changing the scale.
@peril-staging peril-staging bot added the RFC label Jan 28, 2019
@l2succes l2succes changed the title [RFC] Add react-native-web [RFC] Add react-native-svg Jan 28, 2019
@alloy
Copy link
Contributor

alloy commented Jan 28, 2019

We already tried using PNGs instead and pins would look pixelated at higher zoom levels when changing the scale.

I’m not sure I understand this correctly. Is this another issue than the one solved by having various versions of an icon for different device screen scales (or asset bundles)?

@zephraph
Copy link
Contributor

zephraph commented Jan 28, 2019

Last time I talked to @orta about this he seemed fairly strongly against it. A potential alternative here is to convert SVGs over to PNGs via a snapshotting tool. I've actually done this exact thing in the performance-monitor repository. I use the library repng to generate a snapshot of a react component. It's a solution I had to use for emails because they don't support SVGs.

@ashfurrow
Copy link
Contributor

The goal of including SVGs (directly from Zeplin) is definitely a good idea; having one asset that's scalable to any screen is a big win over PNGs. I'd be okay with including an SVG-to-PNG converter in a build process or something, but if this dependency works and it meets our criteria, then it sounds fine to me.

@l2succes
Copy link
Contributor Author

l2succes commented Jan 28, 2019

Yes, I've had this discussion with @orta as well in the past but this use case is slightly different. We need to gradually scale the size our pin icon (max: 1.5x - min: 0.5x) as the user zooms in and out of the map.

I took a video to illustrate what needs to happen. Although it's not currently implemented that's the plan.

So this can't be solved with the standard iOS scales for images ("@3x", "@2x") etc...

@damassi
Copy link
Member

damassi commented Jan 28, 2019

Seems like a great addition barring any gotchas.

@l2succes
Copy link
Contributor Author

@damassi it's my mistake with initially naming the RFC wrong when I first opened it, this has no dependency or anything to do with react-native-web.

@damassi
Copy link
Member

damassi commented Jan 28, 2019

Ah yeah, i just saw that and updated my comment :)

@orta
Copy link
Contributor

orta commented Jan 29, 2019

IMO, this doesn't pull its weight - shipping an entire SVG renderer for a single icon? We should just ship a really big icon and let it get resized correctly by the OS.

@alloy poked me that I should do a second evaluation because this was probably closer to what we want than I think, and he's right.

We've chatted before about what an SVG dependency would look like for us (IRL, so no link) - but a lot of our experience with SVG rendering in iOS apps comes from having a single, massive, comprehensively spec-compliant dependency for rendering an SVG. Which is how everyone does it in native only, but it looks like this is a small and well contained dependency. The native code is readable, and while it's not got tests - it is shipped with Expo by default and I trust that team.

I think this is probably the closest to being what we'd have built out had we done it ourselves, plus it's already 4 years old and has android support which is not what ours is like. So I'm a +1.

@ashfurrow
Copy link
Contributor

@orta cool, I like the updated comment 👍

I wanted to add that SVG's are already a part of our web stack, and adding support for SVG's in Emission would help share more expertise across web and iOS codebases.

@l2succes
Copy link
Contributor Author

Thanks for thorough evaluation everyone and reconsidering @orta 🙏🏾

Based on the feedback, we'll proceed. We actually have about a dozen SVGs for Local Discovery we'll be using this library for, including some of the ones in this screenshots below so it isn't just for one icon to be clear.

screen shot 2019-01-29 at 3 25 48 pm

@zephraph
Copy link
Contributor

Well, this'll make integrating palette icons easy breezy. Go team! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants