Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Updated to use styled QR codes #9155

Closed
wants to merge 4 commits into from

Conversation

ckelwin
Copy link

@ckelwin ckelwin commented Sep 24, 2023

Fixes Issue

Closes #8640

Changes proposed

  • Added styled-qr-code library
  • Removed qrcode.react from package
  • Updated the generated qr with some basic styling

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Updated styled QR:
image

Downloaded qr code still works:
biodrop-eddiejaoude

Note to reviewers

  • Currently, the image displayed in qr code is currently hardcoded. Would be something we want to make configurable for users when editing their profile. A separate issue?
  • Not sure if this requires any documentation changes.
  • I've added some default parameters to the QRStyledCanvas to help developers identify what are some of the available options but it won't be as good as what's possible in typescript. Any suggestions?
    https://github.com/ckelwin/BioDrop/blob/8640-styled-qr-codes/components/QRStyledCanvas.js#L4-L18

@github-actions github-actions bot added issue linked Pull Request has issue linked dependencies Pull requests that update a dependency file and removed issue linked Pull Request has issue linked labels Sep 24, 2023
@amandamartin-dev
Copy link
Contributor

amandamartin-dev commented Sep 24, 2023

Hello! In taking a look at this, I have some concerns about styled-qr-code repo as it appears to be no longer actively maintained. There are a few open issues and 1 PR request that was responded to, but never merged and some comments asking if this is still acitve.

If we are adding this to the project, it seems we will need to commit to updates ourselves so may want to consider another path. Thoughts @eddiejaoude ? I checked through the conversation in the issue where this was also brought up, but unclear if this repo was approved for use

@SaraJaoude SaraJaoude added issue linked Pull Request has issue linked and removed dependencies Pull requests that update a dependency file labels Sep 25, 2023
@eddiejaoude eddiejaoude changed the title Fixes EddieHubCommunity/BioDrop#8640 Updated to use styled QR codes. Updated to use styled QR codes Sep 25, 2023
@eddiejaoude
Copy link
Member

I agree @amandamartin-dev the styled-qr-code repo does look like it is no longer maintained which is a concern - we didn't agree a library, but we definitely need to select one that is still maintained otherwise it could create problems for our project

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

The styled-qr-code doesn't look like it is maintained, are there other alternatives that are still active?

@Dun-sin
Copy link
Contributor

Dun-sin commented Sep 25, 2023

The styled-qr-code doesn't look like it is maintained, are there other alternatives that are still active?

Found this, looks good and is still maintained, and it's better because it's made for nextjs
https://www.npmjs.com/package/next-qrcode

@ckelwin
Copy link
Author

ckelwin commented Sep 26, 2023

Found this, looks good and is still maintained, and it's better because it's made for nextjs https://www.npmjs.com/package/next-qrcode

Thanks for the suggestion. I'll look into this library.

@ckelwin
Copy link
Author

ckelwin commented Sep 28, 2023

I've created a new PR #9196 using the next-qrcode library. There are less options to configure the qrcode but it still works. Most noticeably will be the lack of margin around the logo. Can close this PR if the team decides to go with the other one instead.

Copy link
Member

@sital002 sital002 left a comment

Choose a reason for hiding this comment

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

Since you have created a new PR you can close this pull request.

@SaraJaoude
Copy link
Member

Since you have created a new PR you can close this pull request.

@sital002 closing duplicate PRs would usually be the way forward. However in this case the PRs are not duplicates but different alternatives offered by the author - so we will leave both PRs open for now.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Oct 5, 2023
@eddiejaoude
Copy link
Member

Thank you @ckelwin for the great work and awesome collaboration everyone! 👍 But I think we should close this for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] styled QR codes
6 participants