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

Front end updates #22

Merged
merged 4 commits into from
Oct 23, 2018
Merged

Front end updates #22

merged 4 commits into from
Oct 23, 2018

Conversation

dongbohu
Copy link
Contributor

@dongbohu dongbohu commented Oct 19, 2018

This PR addresses issue #21. It is much larger than I expected, mainly because many front end packages (including the seed project ngbp itself) are too old.

All changes have been tested on a local web server.

@ghost ghost assigned dongbohu Oct 19, 2018
@@ -223,11 +223,14 @@ angular.module('tribe.gene.search', [
$scope.found = $scope.results.found;
var begin, end;
$scope.updatePage = function(page) {
begin = ((page-1)*3), end=begin+3;
Copy link
Contributor Author

@dongbohu dongbohu Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change on this file to make jshint happy. All the others are coding style tweaking, mostly done by emacs editor automatically.

@@ -1,7 +1,6 @@
angular.module( 'tribe', [
'ngCookies',
'templates-app',
'templates-common',
Copy link
Contributor Author

@dongbohu dongbohu Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is removed because the new building procedure wouldn't generate this empty module any more. All the other changes are done by emacs to remove trailing spaces.

Copy link

@arielsvn arielsvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link

@kurtwheeler kurtwheeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like google analytics might be broken by this? Can that be tested?

Also the secrets.ini is in gitignore but referenced by the rest of the code. Is there any way that file is being tracked or anything?

@@ -280,8 +286,6 @@
#'release': raven.fetch_git_sha(dirname(__file__)),
}

# GOOGLE ANALYTICS
GOOGLE_ANALYTICS_KEY = secrets.get('ga', 'GOOGLE_ANALYTICS_KEY')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You moved GA_CODE but completely removed this var. Was this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secrets.ini defined GOOGLE_ANALYTICS_KEY, and production.ini defined GA_CODE. Both of them refer to the same value. But Django is using GA_CODE only (in urls.py). So the one defined in secrets.ini is redundant. That's why I moved GA_CODE from production.ini to secrets.ini.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I have tested the new backend config.

Copy link
Contributor Author

@dongbohu dongbohu Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secrets.ini is ignored because it is supposed to be provided by deployment. As you may remember, it is encrypted and included in the tribe-ec2-deploy repo on bitbucket.

@dongbohu
Copy link
Contributor Author

@kurtwheeler The Tribe backend configuration is a little confusing. I created a new issue #25 in the morning. I will address it after this PR.

@kurtwheeler
Copy link

The only concern I had seems to be a non-issue. :shipit:

@dongbohu dongbohu merged commit 9e871a0 into master Oct 23, 2018
@dongbohu dongbohu deleted the dhu/frontend_update branch October 23, 2018 14:32
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.

None yet

3 participants