tafgeetjs icon indicating copy to clipboard operation
tafgeetjs copied to clipboard

Support plugin options

Open mahmoudzohdi opened this issue 4 years ago • 4 comments

in this PR:

  • support options feature
  • wrap currency option inside the options object (the default still SDG)
  • make the فقط text as an option inside startWith property (the default is فقط)
  • make the لا غير text as an option inside endWith property (the default is لا غير)
  • make the currency and the other options accept null as a value
  • update package version to 2.2.0
  • update README.md file to write the options documentation
  • add .gitignore file to ignore not needed folders/files

mahmoudzohdi avatar Sep 16 '20 23:09 mahmoudzohdi

@mahmoudzohdi Thank you very much for your contribution, Mahmoud! I'll review and merge this as soon as I can.

mmahgoub avatar Sep 20 '20 08:09 mmahgoub

@mmahgoub any updates?

mahmoudzohdi avatar Sep 29 '20 22:09 mahmoudzohdi

Adding options object is a breaking change and it will affect current users of version 2, such change must be in a major release. To add options I suggest we find a way to initialize the Tafgeet object with the configuration needed then let the developer use the function parse in its purest form, adding options every time you want to parse a number is redundant. Also, let us introduce some flexibility so instead of currency in options let's call it symbol

mmahgoub avatar Oct 01 '20 09:10 mmahgoub

I agree with you, it's a breaking change for old users. I suggest changing the computedOptions somehow to support old users if they sent the currency/symbol as a string, like that:

  options = typeof options == 'string' ? {currency: options} :  (options || {})
  this.computedOptions = {
    // first we put the defaults
    currency: "SDG",
    startWith: "فقط",
    endWith: "لا غير",
    // then overwrite with user's options
    ...options
  }

in this way, we didn't break the old user's experience and at the same time introduced the options feature.

and I agree with you, the currency property will be flexible and better if we renamed it to symbol, but you build the currency logic based on that. so, it's better to name it currency because your plugin built based on currencies, I mean if you sent in the currency option a not supported currency the plugin will stop working and you will find errors in the console because you trying to get somethings from not defined currency like that: Cannot read property 'singular' of undefined. or we can enhance it more to make the currency or symbol option to accept predefined currency or any kind of symbol to display

and about the parse function, why should the user call it from outside? it's better to make it work without calling it from outside like that: new Tafgeet(....).

mahmoudzohdi avatar Oct 01 '20 21:10 mahmoudzohdi