gammapy / gamma-sky

Portal to the gamma-ray sky
http://gamma-sky.net/
MIT License
6 stars 3 forks source link

Refactor app from RC4 to Angular 2 Final #57

Closed vorugantia closed 7 years ago

vorugantia commented 7 years ago

In this PR I fully refactored the app to work with the latest Angular update (hopefully stable for the future). I built the app from a fresh start but didn't have to change most of the content itself - I mainly just copied over code. However, here are the major changes implemented in this app:

The code blocks themselves are mostly exactly the same as before - there is no need to review them all in detail. I would mainly just look over how I implemented the modules. After this PR is merged, we should be ready to deploy the app to the website.

cdeil commented 7 years ago

npm start and the webpage works locally for me. This is great!

I don't know why this branch has commits from me and merge commits: https://github.com/gammapy/gamma-sky/pull/57/commits OK to merge as-is this time. For future PRs: please start from master, don't name your feature branch "master", and use rebase instead of merge if you have to sync with the master branch because there have been changes in the meantime.

My main concern here is that ng e2e tests are failing: https://travis-ci.org/gammapy/gamma-sky/builds/171816620#L3851 As I've said in https://github.com/gammapy/gamma-sky/issues/34#issuecomment-238103319 it would be very good to have some automated tests that show that the webpage is basically working (so that we don't do manual re-testing for every future pull request).

If you don't want to fix this here, merging as-is is also OK with me. We still have #34 as a reminder that there should be some working tests. I'll assign #34 to the 0.2 version milestone now.

Otherwise: I'm very short on time and don't know Angular very well, so I'd be OK to merge the code without review.

One thing you could check is whether the commonly used commands developers need are still valid, now that we use Angular 2.1? https://github.com/gammapy/gamma-sky/wiki/Developer-documentation#javascript

vorugantia commented 7 years ago

Don't have time now to fix/figure out the tests. We're still getting an error with travis-ci, even after I commented out the spec.ts files, but I haven't looked at the log yet. I'll just merge this PR for now.