search

vitalets / angular-xeditable

Edit in place for AngularJS
http://vitalets.github.io/angular-xeditable
MIT License
1.91k stars 403 forks source link

Use new angular-bootstrap attribute directive instead of element dire… #630

Closed B3nCr closed 7 years ago

B3nCr commented 7 years ago

…ctive for uib-timepicker in bs-time directive

eugef commented 7 years ago

@B3nCr, @ckosloski what about tests? are they green?

B3nCr commented 7 years ago

Hi

Sorry, i mentioned this in the bug report.

I can't get the tests passing in the master branch so they're still the same here.

I'm wondering if it's because my npm version is not putting files in the same place because I had a problem with grunt build and jsdoc not being found either.

Perhaps you could pull my branch and see if you can run them.

Thanks Ben

On 7 Mar 2017 8:48 a.m., "Eugene Fidelin" notifications@github.com wrote:

@B3nCr https://github.com/B3nCr, @ckosloski https://github.com/ckosloski what about tests? are they green?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vitalets/angular-xeditable/pull/630#issuecomment-284658715, or mute the thread https://github.com/notifications/unsubscribe-auth/ABN9VFK-TPGBS81rvrOWDK3KfCkPhfIZks5rjRnZgaJpZM4MU6X5 .

B3nCr commented 7 years ago

This is my node and npm version for comparison.

D:\dev\Repos\angular-xeditable [squashed-bs-time]> node -v
v6.9.5
D:\dev\Repos\angular-xeditable [squashed-bs-time]> npm -v
4.0.5
eugef commented 7 years ago

@B3nCr npm v2 is required to run build and tests.

B3nCr commented 7 years ago

@eugef Yes, that's what I thought.

Well I'll leave this PR for you to do whatever you choose. I can't see an easy way of installing NPM v2 and v4 side by side. It shouldn't be that difficult for someone to pull my branch and run the tests.

Sorry I couldn't be of more help.

B3nCr commented 7 years ago

Perhaps to make someone else running the tests easier I could create a PR into a different branch in the vitalets repo then it won't be necessary to clone my repo, just checkout a branch.

B3nCr commented 7 years ago

I'll see if I can get Travis CI to build my branch.

eugef commented 7 years ago

Maybe @ckosloski could run the tests agains this PR?

ckosloski commented 7 years ago

All tests pass. I also manually tested all bootstrap directives and those passed as well. This will close #559

eugef commented 7 years ago

Great, then I merge it.

B3nCr commented 7 years ago

Thanks.

I know this is merged but is this an appropriate place to discuss perhaps getting a travis-ci build setup that runs the tests? Is that something that anyone would be interested in?

I've set one up that runs grunt build and it seems to work fine but I'm not sure how to run the tests from the command line.

https://travis-ci.org/B3nCr/angular-xeditable

eugef commented 7 years ago

As far i know tests can be run only in browser. Can Travis-CI do it?

B3nCr commented 7 years ago

I'm not sure yet, it seems like it can but I was hoping there would be a command line way that I could experiment with first.

The tests seem to be using the old angular-scenario e2e testing.

I could take a look at updating the tests to use protractor at the same time, there seems to be a lot more documentation for Travis+Protractor?

Another question, more of a side question. Of I was to start looking at the protractor tests what's the best way to take my repo forwards? I've tried to rebase the vitalets master back into my B3nCr master but now GitHub is telling me they are diverged, the rebase must have created new commits (i thought the point of rebase was that it didn't do that).

Should I just drop my fork and create a fresh fork for future contributions?

On Wed, Mar 8, 2017 at 11:54 AM, Eugene Fidelin notifications@github.com wrote:

As far i know tests can be run only in browser. Can Travis-CI do it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vitalets/angular-xeditable/pull/630#issuecomment-285021964, or mute the thread https://github.com/notifications/unsubscribe-auth/ABN9VOL1Z9YbG-PlJsYGLB48c0w_hySaks5rjpcRgaJpZM4MU6X5 .

ckosloski commented 7 years ago

This is what I do to rebase.

git fetch upstream
git rebase upstream/master

That would be great if you have time to update the tests!!!

B3nCr commented 7 years ago

I take it you manually add the upstream origin which refers to the the source repo of the fork, my local repo only has one remote which is my github repo,

On Wed, Mar 8, 2017 at 3:52 PM, ckosloski notifications@github.com wrote:

This is what I do to rebase.

git fetch upstream git rebase upstream/master

That would be great if you have time to update the tests!!!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vitalets/angular-xeditable/pull/630#issuecomment-285079310, or mute the thread https://github.com/notifications/unsubscribe-auth/ABN9VGwGRoRd3xTwkUKLxHmTn5tdzN7Jks5rjs6pgaJpZM4MU6X5 .

ckosloski commented 7 years ago

I don't remember what I did, but I used those commands after I viewed this stackoverflow