-
Notifications
You must be signed in to change notification settings - Fork 205
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
New WebUI: Update angular and angular-route and add workaround to be compatible #1813
New WebUI: Update angular and angular-route and add workaround to be compatible #1813
Conversation
…test upstream releases)
Oh, forgot: This PR also updates jQuery to the latest upstream release. I did test the new Web UI after the changes:
All functions seem to work as before |
Oops, sorry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look fine. I haven't checked out the code locally though.
Looks like the Travis CI job doesn't even start. Should we merge the PR manually without that check? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes seem fine, Haven't tested it in the real world though.
Travis CI is giving the following error on the site (when i logged in at least):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with others LGTM but I've not tried it.
Just the one possible comment improvement.
// AngularJS 1.6 now uses an '!' as the default hashPrefix, breaking | ||
// our routes. Following lines revert the default to the previous empty | ||
// string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could also add a note saying given this webpage is only used internally, there is no benefit in the web crawler changes.
We could possibly add a TODO to update our URLs to remove this change, but maybe that's not necessary if we're rewriting to a new framework in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this might be useful but since AngularJS (= 1.x) is a dead end, we won't bother for now
You could try going here @kripton and logging into Travis: Maybe this is just a guerrilla attempt to force more people to GitHub Actions? Maybe we should work on this for Linux too:
Quite possibly, the tests won't really check it aside from the JS linting stuff which I guess you've run? Or do a dummy commit and see if it does anything the second time around. |
@peternewman I'm happy to work on a Linux build for GitHub Actions to compliment #1809 and #1812 to fully replace Travis. |
I did. It gave me some "plan exceeded" limit, mainly regarding the "number of users on the plan". However, I am unsure if it meant my personal GitHub account or the OpenLightingProject organization. With all the positive review feedbacks here and since Travis won't catch errors in JavaScript anyways, I'll just merge this one. |
This PR updates angular and angular-route to the latest upstream releases (v1.8.3). However, without changes to our new Web UI, this causes it to break as described in #1782. To fix this, a workaround has been added to restore angular's previous behaviour. See https://docs.angularjs.org/guide/migration#commit-aa077e8
As a bonus, the list of dependencies in
javascript/new-src/package.json
is not sorted alphabetically as is done automatically when adding or updating packages listed therein.This PR also updates the "built" app.min.js and related files.