-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Waiting for JsBarcode 4] New options from element #107
base: master
Are you sure you want to change the base?
Conversation
@@ -1,25 +1,25 @@ | |||
import optionsFromStrings from "./optionsFromStrings.js"; | |||
import defaults from "../options/defaults.js"; | |||
|
|||
function getOptionsFromElement(element){ |
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.
As a suggestion:
function getOptionsFromElement(element) {
const options = {};
const attributes = element.attributes;
for (let i = 0; i < attributes.length; i++) {
const match = attributes[i].name
.match(/(jsbarcode|data)-([A-Za-z0-9-]+)/i);
if (match) {
// Transforms foo-bar to fooBar
const property = match[2].replace(/-[a-zA-Z]/, (val) => val[1].toUpperCase());
options[property] = attributes[i].value;
}
}
// Since all attributes are string they need to be converted to integers
return optionsFromStrings(options);
}
If I understand right, you forgot about conversion of const INT_OPTIONS = [
"width",
"height",
"textMargin",
"fontSize",
"margin",
"marginTop",
"marginBottom",
"marginLeft",
"marginRight"
];
const BOOL_OPTIONS = [
"displayValue",
"flat"
];
// Convert string to integers/booleans where it should be
function optionsFromStrings(options) {
Object.keys(options).forEach(key => {
if (typeof options[key] === "string") { // not sure about this
if (INT_OPTIONS.indexOf(key) !== -1) {
options[key] = parseInt(options[key], 10);
} else if (BOOL_OPTIONS.indexOf(key) !== -1) {
options[key] = options[key] === "true";
}
}
});
return options;
}
export default optionsFromStrings; |
@SanichKotikov Indeed flat should be converted. But doing it that way would violate some important design principles. Primarly the dependency inversion principle. This means that every time an option is added to a barcode symbology the main library has to be updated which should be avoided. But as you noted this is still an option that should be addressed. Maybe by allowing the symbologies to specify its options and the datatype to allow the main part of the library to convert it. |
I see. Good point about DI principle. I have a crazy idea https://jsfiddle.net/o6cbz9na/1/. If you need to add a new option, you will do it only in |
That's all good! But the problem still remains that new (barcode specific) options does need to be edited in to work. This implementation is much nicer for the library options and should probably be implemented for those 😄 |
@lindell just a thought: why don't we apply |
@lindell any news on this? It fixes a lot of stuff for options, desperately need it!! 😄 |
@garygreen I think I will realease the this when #171 i done which is hopefully pretty soon. But what do you want to do exactly? There is probably a pretty easy workaraound that is just not as pretty. |
Probably is an easy workaround, I guess I'll have to initialise the barcodes manually - still would like to give options in a |
@garygreen You can already, with all options except the barcode specific ones. So basically the |
Will allow barcode specific options to be specified through html parameters.
This does also force the use of
some-option
when you want to specifysomeOption
which will break backwards compatibility.Scheduled for JsBarcode 4 release.