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

alert controller #252

Merged
merged 2 commits into from
Jun 11, 2017
Merged

alert controller #252

merged 2 commits into from
Jun 11, 2017

Conversation

hfwittmann
Copy link
Contributor

Hi Stephen,

so here it is:

the alert controller is incorporated now.

Hopefully everything works fine.

Cheers, Felix

PS: May I draw your attention to this project: https://github.com/stonelasley/ionic-mocks

I have used a snippet from that project (it is marked). Maybe it would make sense to use this project?

@lathonez
Copy link
Owner

Thanks Felix.

Please see the above CI failures. Travis is one to look at - Appveyor has a bit of a mind of it's own.

I can see the following issues:

  • A unit test is failing due to no provider for alert controller
  • various linting issues
  • An E2E test is failing

If you can resolve these we can get it merged in.

Cheers

@hfwittmann
Copy link
Contributor Author

hfwittmann commented Jun 10, 2017

Hi Stephen,

thanks for the info.

I have now corrected

a) the linting issues
b) the e2e test: page2.e2e-spec.ts

As for the unit tests, one of the files had not been pulled.

I have now doubly checked everything works as expected, by pulling directly from my fork like this:

This step fails initially, and has to be run again.

Remark: I think this might be an ionic issue. ( It complains about the postinstall script failing). I think there was an issue with the build last time....

  • npm run test
  • npm run test-coverage
  • npm run e2e

So fingers crossed...

Cheers, Felix

@lathonez lathonez merged commit 310a2e9 into lathonez:master Jun 11, 2017
@lathonez lathonez mentioned this pull request Jun 11, 2017
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