Skip to content

Add useQRCode hook #134

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
Mar 9, 2020
Merged

Add useQRCode hook #134

merged 2 commits into from
Mar 9, 2020

Conversation

maddie927
Copy link
Member

@maddie927 maddie927 commented Mar 4, 2020

I ended up making this a hook which renders an svg in order to package up the rendering and "save on click" behavior into a single function without exposing the internal bits like refs. It leaves the style open for the consumer to control.

image

@maddie927 maddie927 self-assigned this Mar 4, 2020
Copy link
Contributor

@arthurxavierx arthurxavierx left a comment

Choose a reason for hiding this comment

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

This hook+component pair seems a bit too specific for me, especially because of the saveOnClick effect the hook returns. I feel like a more general approach would be to have it return an effect that produces the SVG blob/data URL, so the consumer can choose what to do with it, be it saving to a file, printing it, or whatever.

, level: errorCorrectLevelToString level
, renderAs: "svg"
, xmlns: "http://www.w3.org/2000/svg"
, size: 126
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the size should be configurable here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up removing it which leaves the hard-coded sizing off of the svg element. The size can still be specified with styles, allowing other units or relative sizing.

@maddie927
Copy link
Member Author

Hmm, maybe. You mean more like base64url <- useQRCode val? You'd then need to set it as an img src if you wanted to render it, instead of treating it like jsx or html.

@arthurxavierx
Copy link
Contributor

I'd actually thought of still returning the qrcode as JSX but at the same time allowing for consumers to use the data URL in ways other than downloading.

@maddie927
Copy link
Member Author

Got it. I'm going to try a variation in a bit using a different library and see how that feels. I like the library better overall, especially when we get to future requirements like logo overlays.

@maddie927
Copy link
Member Author

ready to re-review

@kimispencer
Copy link
Contributor

Feel free to ignore this, but in order to test the UI capabilities of this component I added an additional example in kimi/qrcode-example. UI wise this is looking solid to me.

@maddie927 maddie927 merged commit 102041d into master Mar 9, 2020
@maddie927 maddie927 deleted the madeline/qrcode branch March 9, 2020 23:32
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.

4 participants