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

Added minified version for all cultures; Changed de-DE currency symbol #4

Merged
merged 2 commits into from
Aug 14, 2018

Conversation

frankiDotNet
Copy link
Contributor

  • Changed 'de-CH' currency symbol to 'CHF'
  • Added minified version for all cultures

- Changed 'de-CH' currency symbol to 'CHF'
Copy link
Owner

@thorn0 thorn0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committing minified files is a bad practice. I'm okay with having such a file in the package, but not in the repo. Please instead of this file, commit a change to gruntfile so that when we run grunt build, this file will be built too. Also, please note that every culture file has its own copy of the UMD boilerplate code whereas in the combined file only one copy of it would suffice:

;(function (global, factory) {
   typeof exports === 'object' && typeof module !== 'undefined'
       && typeof require === 'function' ? factory(require('../sffjs')) :
   typeof define === 'function' && define.amd ? define(['../sffjs'], factory) :
   factory(global.sffjs)
}(this, function (sffjs) { 'use strict';
    sffjs.registerCulture({ ... });
    sffjs.registerCulture({ ... });
    // ...
}));

@frankiDotNet
Copy link
Contributor Author

I am not familiar with grunt build process. I would take care of this, but to stay generic all files should be untouched and the build task should extract the sffjs.registerCulture part concat all of them into the above written fuction that you described. That right ?

For now it is deleted, because the minified file should be part of build process..
@thorn0
Copy link
Owner

thorn0 commented Aug 14, 2018

Right. I'm going to merge the CHF change now. Please submit a separate PR for that combined culture file.

BTW, overall, it's not a good idea to mix multiple unrelated changes in one PR. It's always better to submit separate PRs in such a situation.

@thorn0 thorn0 merged commit cfca2c6 into thorn0:master Aug 14, 2018
@frankiDotNet
Copy link
Contributor Author

Thanks! I will take care next time..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants