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

Rotate dots #110

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Rotate dots #110

merged 1 commit into from
Oct 11, 2024

Conversation

dmitrystas
Copy link
Contributor

#49

@HenkVanMaanen
Copy link
Contributor

@dmitrystas this can be fixed too by the following code, it needs to be fixed in both QRSVG.ts and QRCanvas.ts:

WRONG:
dot.draw( xBeginning + i * dotSize, yBeginning + j * dotSize,

FIX:
dot.draw( yBeginning + j * dotSize, xBeginning + i * dotSize,

@HenkVanMaanen
Copy link
Contributor

See fix here: https://github.com/sallandpioneers/qr-code-styling

This fork will be maintained until further notice by Salland Pioneers (business).

@NickStull
Copy link

NickStull commented Jul 29, 2022

@dmitrystas this can be fixed too by the following code, it needs to be fixed in both QRSVG.ts and QRCanvas.ts:

WRONG: dot.draw( xBeginning + i * dotSize, yBeginning + j * dotSize,

FIX: dot.draw( yBeginning + j * dotSize, xBeginning + i * dotSize,

@HenkVanMaanen This works for two of the styles but not the others. It's a great fix in general, but it breaks the dot designs for any design other than square and circle. This is because you are transposing them after the dots are designed. You can see this about 12 lines up.

const dot = new QRDot({ context: canvasContext, type: options.dotsOptions.type });

This is where it styles the dot. Circle and Square don't matter, but the other designs like rounded and classy are designed depending on their neighbor.

I'm trying to work through the code to see if this can be transposed earlier in the code. No luck so far understanding exactly how it works. But I'll reply when I do.

Edit: figured it out. Turns out it was only affecting the rounded and extra rounded dot styles. I'll post the fix when I'm back in the office. It's definitely a bit of a hacky fix, but it works. Im not exactly sure of what the actual issue is and why it only affects those two styles. I have a hunch of where it's broken, but I'm having a hard time understanding the canvas and SVG drawing as it's not an area I have ever worked in.

@KilianB
Copy link
Contributor

KilianB commented Aug 26, 2022

@NickStull do you still intend on publishing your fix?

@KilianB KilianB mentioned this pull request Aug 26, 2022
KilianB added a commit to KilianB/styled-qr-code that referenced this pull request Aug 27, 2022
@madmacc
Copy link

madmacc commented Sep 19, 2022

@NickStull Any update on publishing this?

@NickStull
Copy link

@KilianB @madmacc I do have it published but to be honest I've made other changes that I'm not comfortable other people using, only because I'm not confident I didn't introduce bugs. I have not been able to thoroughly test. I'd genuinely feel bad if I caused issues for other devs because I don't fully know if my fix is right.

with that said, if you'd like to try it out here it is: https://github.com/PlasticPrintersMN/pp-qr-code-styling you can see my fixes in the commits ahead. I still need to change the readme to reflect that this is not the original repo. Basically user beware. I'm still learning.

@madmacc
Copy link

madmacc commented Sep 19, 2022

@NickStull Thanks, understood. I just discovered this issue and I have until the end of the week to resolve it so looking at all options!

@madmacc
Copy link

madmacc commented Sep 19, 2022

@NickStull based on your comment above do you still think @HenkVanMaanen's version would work if just used with square style dots?

@NickStull
Copy link

@madmacc you'll notice, after getting the dots rotation corrected it messed up the cutout for adding an image to the center. That fix is in here as well. Just to say it outloud, I'm not convinced this is the proper fix for the actual issue. There is an issue with how it's building the qr, but I digress, so far it seems to work.

@NickStull
Copy link

@NickStull based on your comment above do you still think @HenkVanMaanen's version would work if just used with square style dots?

@madmacc yes , their fix works if using square only, but you will still need the image cutout fix if you plan to put a logo in the middle.

@madmacc
Copy link

madmacc commented Sep 19, 2022

Another option I saw was this one:
https://github.com/KilianB/styled-qr-code
See: #105 (comment)

I will update with what I end up using.

@madmacc
Copy link

madmacc commented Sep 20, 2022

@KilianB @madmacc I do have it published but to be honest I've made other changes that I'm not comfortable other people using, only because I'm not confident I didn't introduce bugs. I have not been able to thoroughly test. I'd genuinely feel bad if I caused issues for other devs because I don't fully know if my fix is right.

with that said, if you'd like to try it out here it is: https://github.com/PlasticPrintersMN/pp-qr-code-styling you can see my fixes in the commits ahead. I still need to change the readme to reflect that this is not the original repo. Basically user beware. I'm still learning.

I have tried installing this using npm install PlasticPrintersMN/pp-qr-code-styling but it does not create the lib folder just the root files. There is a good chance I am doing something wrong as I have only every installed from www.npmjs.com before.
TinyTake20-09-2022-03-00-33

I did try @HenkVanMaanen's fork (npm install sallandpioneers/qr-code-styling) and this does work well with Android for a square with or without image QR code.

@KilianB's fork requires a later version of typescript than I have in my project so I can't test that.

@madmacc
Copy link

madmacc commented Jan 9, 2023

Anyone keen on having a go at fixing this?
Happy to help test but I think it is beyond me to make the fix.

ChielTimmermans added a commit to sallandpioneers/qr-code-styling that referenced this pull request Jul 11, 2024
* update dependencies

* make examples runnable again

* proper fix for mirrored qr code and all styles fixes kozakdenys#105, fixes kozakdenys#49, fixes kozakdenys#110

* do not distribute packages via git anymore. prepare npm release

* target es2017

* added toDataUrl function and improved typescript types

* prepare and publish version 1.0.0

* security patch

---------

Co-authored-by: Kilian Brachtendorf <Kilian.Brachtendorf@t-online.de>
@kozakdenys kozakdenys changed the base branch from release_1.6.0 to master October 11, 2024 09:56
@kozakdenys kozakdenys merged commit 408aa73 into kozakdenys:master Oct 11, 2024
@github-actions github-actions bot mentioned this pull request Oct 11, 2024
@kozakdenys
Copy link
Owner

Finally merged it, you can find it here https://github.com/kozakdenys/qr-code-styling/releases/tag/v1.6.1
Thanks @dmitrystas for your PR and sorry folks you waited for a merge for so long, I promise not to leave you anymore 😉

@kozakdenys
Copy link
Owner

The v1.6.1 didn't fix the version correctly, so I released new version v1.7.1 with correct fix

@kozakdenys kozakdenys added the reviewed Temporary label to review all open and closed PR label Oct 14, 2024
@madmacc
Copy link

madmacc commented Oct 14, 2024

@kozakdenys thanks for that! I will give it a try in a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed Temporary label to review all open and closed PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants