Skip to content

Conversation

@gregziegan
Copy link
Contributor

@gregziegan gregziegan commented Aug 11, 2016

Pre-merge checklist

  • accompanying tests are written and passing
  • npm run install passes with no errors
  • npm run lint passes with no errors

Description of changes
BR is now preserved

Reason for changes or related GitHub issue
#21

@aem

@codecov-io
Copy link

codecov-io commented Aug 11, 2016

Current coverage is 94.82% (diff: 100%)

Merging #22 into master will increase coverage by 0.64%

@@             master        #22   diff @@
==========================================
  Files             4          4          
  Lines           172        174     +2   
  Methods          11         11          
  Messages          0          0          
  Branches         27         28     +1   
==========================================
+ Hits            162        165     +3   
+ Misses           10          9     -1   
  Partials          0          0          

Powered by Codecov. Last update b57cebd...9635d27

@gregziegan
Copy link
Contributor Author

When npm installing my fork currently, the gulp build does not run the gulp script to build the dist directory. Any idea why that would be?

@aem
Copy link
Owner

aem commented Aug 12, 2016

I personally believe that build steps shouldn't slow down the installation process, so I don't run gulp as a part of a prepublish script, rather I just have a local alias for publishing to NPM that runs gulp; npm publish

@aem
Copy link
Owner

aem commented Aug 12, 2016

Can you possibly write a unit test that accompanies this change? It's a good catch and something that I don't want to regress

@gregziegan
Copy link
Contributor Author

We're good to go 👍 It's a simple test but it does fail before my patch

@aem
Copy link
Owner

aem commented Aug 13, 2016

@thebritican looks solid! merging and will publish 0.1.6 in a bit

@aem aem merged commit 54157c0 into aem:master Aug 13, 2016
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.

3 participants