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

Example: how to download the qr code as an image #17

Closed
wants to merge 2 commits into from
Closed

Example: how to download the qr code as an image #17

wants to merge 2 commits into from

Conversation

keyserfaty
Copy link

@keyserfaty keyserfaty commented Sep 6, 2016

Hi! I am using your component and I found myself needing to download the qr code as an image. Maybe someone else might need to do such a thing too.
I added an example in the README on how to do it using the ref prop from the component.

Feel free to reject it if you don't like the idea!
Thanks for the work btw :)


## Downloading the *qr code* as an image

The *qr code* generated by the component is a [canvas](http://www.w3schools.com/html/html5_canvas.asp). If you need to create a button to download the qr code image you can use the component's `ref` prop like this:
Copy link

Choose a reason for hiding this comment

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

Let's change w3schools link to an equivalent MDN link? It has better content.

Copy link

Choose a reason for hiding this comment

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

Ref is technically not a prop. Maybe "component's ref callback attribute" would be better. That's how we call them in React docs.

@gaearon
Copy link

gaearon commented Sep 11, 2016

Hmm. I took another look at the README and I now realize this is probably not part of public API. Specifically, reading e.refs.canvas is likely not officially supported. Generally you should only access your own refs, and reading deeper refs is breaking encapsulation and generally not considered part of component public API.

I think that before this can be merged, there should be an official API for getting the URL. For example, it could be an instance method on the component. This would require that @zpao is interested in adding this API.

I'm not a maintainer btw, just happened to be passing by and thought I'd leave a few comments while @zpao is busy with other projects. I'm not sure how to move this forward but I hope the above comments were at least a tiny bit helpful.

@zpao
Copy link
Owner

zpao commented Sep 12, 2016

Hey @keyserfaty, sorry for taking so long to get back to you! And thanks @gaearon for taking the time to look at it! I'll second most of what he said, and most importantly, the public API part. I think it's awesome that you found a way to make this work without needing an official API but it's not super future proof. If React stops supporting string refs, or we didn't render a <canvas> directly in this component, it would stop working. Also, there are potential timing issues - since we actually update the DOM in componentDidMount/Update, if you access the ref at the wrong time it might not have the correct data yet. So I think a public API is in required.

I think the best API might be a callback prop (maybe named something like onDataURLChange) that we call after we update the canvas data. This would avoid the timing issue and would actually end up with pretty much the same example component you already wrote (since you used callback refs 🎉). We might need to decide if we want to support non-default values for toDataURL, but I think we could probably just stick to the defaults for now and if it becomes an issue, add more props later.

Would you be interested in adding that API? (or if you have other API ideas, let me know and we can chat). Feel free to bikeshed on an API name too, I'm not attached to anything yet.

@keyserfaty
Copy link
Author

Hey! Thanks for the comments. I totally agree with both. I would love to add that to the API. I will be looking at it this week and try to make a new PR by the end of the week.
Thanks for the extensive comments. Feel free to reject to PR :)

@keyserfaty keyserfaty closed this Nov 5, 2016
@zpao zpao mentioned this pull request Apr 28, 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.

3 participants