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

Add files option for npm, fix indents on tests #1883

Merged
merged 1 commit into from
May 14, 2016
Merged

Add files option for npm, fix indents on tests #1883

merged 1 commit into from
May 14, 2016

Conversation

vsn4ik
Copy link
Member

@vsn4ik vsn4ik commented May 7, 2016

  1. Fix references in docs
  2. Update change events in docs
  3. Fix idents on tests
  4. Move rows from .npmignore to .gitignore
  5. Add files option on package.json
  6. Use https when possible
  7. ...

@@ -59,7 +59,6 @@ module.exports = function(grunt){
},
concat: {
options: {
banner: '<%= banner %>',
Copy link
Member

Choose a reason for hiding this comment

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

why did you remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

For unification banner for js and css files.

Copy link
Member

Choose a reason for hiding this comment

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

ah yes I see, it's now in the usebanner task!

@acrobat
Copy link
Member

acrobat commented May 7, 2016

Thanks @vsn4ik for the cleanup! Can you restore these files:

  • .npmignore (this was added in a pr for a specific case, so we should keep it for now (possible cleanup in 2.0)
  • tests/assets/jquery-2.2.3.min.js (I've added this file yesterday to start some work on jquery 1 and 2 test suites)

Also please take a look at the travis build! Thanks!

@vsn4ik vsn4ik changed the title Use nodejs 6, add files option for npm, fix indents on tests Add files option for npm, fix indents on tests May 7, 2016
@vsn4ik
Copy link
Member Author

vsn4ik commented May 10, 2016

@acrobat I'm not going to restore unwanted files and unused libraries.

@acrobat
Copy link
Member

acrobat commented May 11, 2016

Ok, I will add the new jquery file in a separate PR for extra tests. But the .npmignore file was added to fix #1742, so we should atleast keep it until 2.0 (then we can remove it and fix things an other way)
What you can do is move the content from the .npmignore to the gitignore file as npm uses that file if a npmignore is missing! (ref)

I see you did a lot of small changes to the main js file, the changes look good on first sight. Can you guys also take a look @hebbet @Azaret, thanks!

@acrobat
Copy link
Member

acrobat commented May 14, 2016

@vsn4ik thanks for fixing the comments! Is it maybe possible to move the changes to bootstrap-datepicker.js to a separate pull request. Other changes can stay in this PR and are ready for a merge!

@vsn4ik
Copy link
Member Author

vsn4ik commented May 14, 2016

Done.

@acrobat
Copy link
Member

acrobat commented May 14, 2016

Thanks @vsn4ik! Will review the other PR soon!

@acrobat acrobat merged commit 599e4c7 into uxsolutions:master May 14, 2016
@acrobat acrobat added this to the 1.7.0 milestone May 14, 2016
@vsn4ik vsn4ik deleted the add_npm_files_option branch May 14, 2016 14:59
@acrobat acrobat mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants