-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add EAN & UPC codes to loyalty cards app #3465
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
Conversation
|
Looks good to me! Shall wait for @glemco to review. I notice there's licenses in some of the source files, @gfwilliams do you remember what your thoughts were on this? |
|
This was @gfwilliams reply in the original Pull request: Originally posted by @gfwilliams in #2977 (comment) So I just added the same license comments on the new files (They are from the same library as the other ones, but slightly modified to work on Espruino) |
|
Thanks for digging that out, in that case let's merge once @glemco's taken a gander |
glemco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot test it at the moment, but the code looks good, thanks.
I would say if the app's size is not increasing drastically (and it shouldn't) it's all right.
Regarding the codes unused by the loyalty card app, do you see a use case in having them? Can the user just test them by manually changing the files on the bangle? Unless you deem them useful for whatever reason, I wouldn't include them, but it's up to you
|
Thanks! Yes, I've very happy with the licences as-is - especially as they're MIT which fits with the main repo anyway. I guess my only concern is it's a lot of different files for one app - it's fine here, but in general I'd err towards trying to put more stuff in each file and have less files - the more files on the device the slower file accesses get, so if everyone did this it'd have a big impact. |
|
I've removed the EAN 2 and 5, as they are not supported by the loyalty cards app and removes some files. @gfwilliams I can also inline the constants from the constants file, they seem to be used in a single file only now, that would be one file down. |
|
Thanks! I think putting the constants in the file they're used in would be good, but I wouldn't bother with the rest... It's not a huge problem for it in one or two apps where it makes sense, it's just that we don't want everyone to do it. The other option for some of these is to use tools to roll all the files into one, but that's not ideal either as it stops others from contributing unless they have everything downloaded and running locally with whatever toolchain is used. |
|
Moved the constants to the files they are used in, only 6 new files left now. The gadget bridge pull request to allow the new codes is at https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/3844 |
|
Great - thanks! Merging now :) |
Adding EAN & UPC Barcodes to @glemco cards app.
EAN & UPC barcodes are based on the same https://github.com/lindell/JsBarcode library that was used for the existing codes.
I've added all EAN codes, but EAN 2 and 5 are not supported by the loyalty card app, so I don't know if we want them in, they are very small, but probably not much used for loyalty cards as there's not much information in them.
The gadget bridge pull request to allow the new codes is at https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/3844