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

fix: qrcode text size limit #6140

Merged
merged 4 commits into from
Aug 14, 2023
Merged

fix: qrcode text size limit #6140

merged 4 commits into from
Aug 14, 2023

Conversation

sreehari2003
Copy link
Contributor

@sreehari2003 sreehari2003 commented Aug 7, 2023

Change Summary

Change type

  • feat: (new feature for the user, not a new feature for build script)
  • fix: (bug fix for the user, not a fix to a build script)
  • docs: (changes to the documentation)
  • style: (formatting, missing semi colons, etc; no production code change)
  • refactor: (refactoring production code, eg. renaming a variable)
  • test: (adding missing tests, refactoring tests; no production code change)
  • chore: (updating grunt tasks etc; no production code change)

Provide summary of changes.

reduced the text limit on qrcode

Anything for maintainers to be made aware of

final output

image

Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

I don't think this is the complete fix for the issue. This PR only prevents the cell to not show too many chars for QR code while they are possible to store in QR code standard. A QR code can store up to 7089 digits or 4296 characters and the current approach is limiting down to 62 characters.

Currently we're using version 4 in qrCodeOptions which is pretty small. That's the reason why we cannot store too many chars. There're options we can configure the version (See here - we can bump the version to allow more characters).

Therefore, I think the better approach would be dynamically setting version in qrCodeOptions based on the length of the data in the cell, i.e. qrValue?.value.length. The more characters the data has, the higher version it requires. I'll leave you to do the math.

@sreehari2003
Copy link
Contributor Author

I don't think this is the complete fix for the issue. This PR only prevents the cell to not show too many chars for QR code while they are possible to store in QR code standard. A QR code can store up to 7089 digits or 4296 characters and the current approach is limiting down to 62 characters.

Currently we're using version 4 in qrCodeOptions which is pretty small. That's the reason why we cannot store too many chars. There're options we can configure the version (See here - we can bump the version to allow more characters).

Therefore, I think the better approach would be dynamically setting version in qrCodeOptions based on the length of the data in the cell, i.e. qrValue?.value.length. The more characters the data has, the higher version it requires. I'll leave you to do the math.

I couldn't find anyway to calculate the char and version connection

length vs version relation looks like

// represent  length:version
const qrCodeVersionConfig = {
  62: 4,
  87: 5,
}


@wingkwong
Copy link
Member

@sreehari2003 Just skimmed through the docs a bit, options.version actually is not required for our case. If it's not specified, then the more suitable value will be used. They already do the math for you :D. Ref: (https://github.com/soldair/node-qrcode#qr-code-capacity)

image

@sreehari2003
Copy link
Contributor Author

@sreehari2003 Just skimmed through the docs a bit, options.version actually is not required for our case. If it's not specified, then the more suitable value will be used. They already do the math for you :D. Ref: (https://github.com/soldair/node-qrcode#qr-code-capacity)

image

is this qr is scannable ? i tried scanning some large text encoded qr and phone is not able to scan it but small qr is working , I have updated the pr

@wingkwong
Copy link
Member

is this qr is scannable ? i tried scanning some large text encoded qr and phone is not able to scan it but small qr is working , I have updated the pr

Yes. It'll link to search engine with the given text. By the way, as you may have noticed the CI/CD failed, since the version is dynamic now, please update the tests/playwright/tests/db/columns/columnQrCode.spec.ts accordingly.

@sreehari2003
Copy link
Contributor Author

is this qr is scannable ? i tried scanning some large text encoded qr and phone is not able to scan it but small qr is working , I have updated the pr

Yes. It'll link to search engine with the given text. By the way, as you may have noticed the CI/CD failed, since the version is dynamic now, please update the tests/playwright/tests/db/columns/columnQrCode.spec.ts accordingly.

i checked the code but I don't see anywhere in the code where version is depended on , this is my first time working withe test cases

@wingkwong
Copy link
Member

@sreehari2003 Previously the version was hardcoded to version: 4. Since the version in this PR is dynamic, the library determines the version. Therefore, the value will be different. You just need to update the expected value.

@wingkwong wingkwong merged commit 4af03cb into nocodb:develop Aug 14, 2023
15 checks passed
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.

🐛 Bug: QR-Code limited number of characters
2 participants