espruino / BangleApps

Bangle.js App Loader (and Apps)
https://banglejs.com/apps
MIT License
498 stars 1.17k forks source link

Add EAN & UPC codes to loyalty cards app #3465

Closed esdeboer closed 5 months ago

esdeboer commented 5 months ago

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

bobrippling commented 5 months ago

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?

esdeboer commented 5 months ago

This was @gfwilliams reply in the original Pull request:

          Wrt licenses, please could you add a comment at the top of each of the JS files (just what they're based on and what the license is) - I think that'd be ok for us with it in the README as well.

Originally posted by @gfwilliams in https://github.com/espruino/BangleApps/issues/2977#issuecomment-1717180679

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)

bobrippling commented 5 months ago

Thanks for digging that out, in that case let's merge once @glemco's taken a gander

gfwilliams commented 5 months ago

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.

esdeboer commented 5 months ago

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. The encode function could also be moved to the UPC file so the encode file can be removed, would mean EAN class uses it from the UPC class, which is less nice, but maybe OK as they are the same family of codes anyways.

gfwilliams commented 5 months ago

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.

esdeboer commented 5 months ago

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

gfwilliams commented 5 months ago

Great - thanks! Merging now :)