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

Ft/backend #150

Closed
wants to merge 2 commits into from
Closed

Ft/backend #150

wants to merge 2 commits into from

Conversation

NyanHelsing
Copy link
Contributor

No description provided.

@NyanHelsing NyanHelsing changed the base branch from develop to master April 11, 2017 19:09
Copy link
Contributor

@samanehsan samanehsan left a comment

Choose a reason for hiding this comment

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

Quick first pass -- please go ahead and make these changes (almost all removing local files) so that the PR is easier to review

@@ -0,0 +1 @@
sharedash
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should be in the .gitignore

@@ -0,0 +1,29 @@
# -*- coding: utf-8 -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

This migration file shouldn't be committed

@@ -0,0 +1,23 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.10.3 on 2017-03-29 17:30
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, this migration file shouldn't be committed

@@ -0,0 +1,56 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.10.3 on 2017-03-29 17:30
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this migration file as well

@@ -1,8 +1,8 @@
DATABASES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Local setting should be in the .gitignore

@@ -408,8 +406,10 @@ export default Ember.Component.extend({
picking: false,

init() {

debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the stray debugger statement

//this.sendAction('refreshWall');
debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the stray debugger statement

@@ -8,6 +8,10 @@

class Dashboard(Model):

id = CharField(
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed earlier, please move the dashboard and widget models into the api app

class Widget(Model):

title = CharField(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is title the same as chartType?

@@ -3,8 +3,8 @@ import JSONAPIAdapter from 'ember-data/adapters/json-api';
import config from '../config/environment';

export default JSONAPIAdapter.extend({

host: config.APP.GRANTS_BACKEND,
host: 'http://localhost:8000',
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of defining the host in each adapter, you can define it once in the application adapter and extend it here and in the dashboard adapter

@samanehsan
Copy link
Contributor

Please bring this PR up to date with master :octocat:

@samanehsan
Copy link
Contributor

Done reviewing @birdbrained! ☀️

@samanehsan
Copy link
Contributor

Continued in #154

@samanehsan samanehsan closed this Apr 18, 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