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

Integration-report - Work in progress #125

Merged
merged 6 commits into from
Jan 26, 2017
Merged

Integration-report - Work in progress #125

merged 6 commits into from
Jan 26, 2017

Conversation

josigama
Copy link
Contributor

Integração front-end e back-end da funcionalidade para reportar um risco

@viniciusgama
Copy link
Contributor

@ebragaparah can you check if I messed something after the merge? all things you did in the other PR are still in place?

@ebragaparah
Copy link
Contributor

You mean, should I check if the code I put in my PR is present in this one? Have you already merged this version to my latest code? I couldn't find my changes here.

@viniciusgama
Copy link
Contributor

exactly @ebragaparah

@viniciusgama
Copy link
Contributor

Algumas tarefas pendentes:

  • Config Management
  • Deployar para um ambiente
  • Integrar front e back neste ambiente

@ebragaparah
Copy link
Contributor

All my changes are there after your changes, folks!

if (!found) {
$scope.placeDetails.occurrences.push(addPlaceFromForm($scope.report));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked it! I agree with you folks that this statement is just bringing complexity to this controller since we don't need to group the occurrences here.

$scope.placeDetails.occurrences.push(addPlaceFromForm($scope.report));
}
}
$http.post(ApiEndpoint.url, addPlaceFromForm($scope.report), {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's a good ideia to delegate this $http.post for an specific service that can be injected here. Since it's not an urgency, we can add this to our tech debit wall (I took notes on this).

@@ -39,7 +39,8 @@
"moment": "^2.17.1",
"phantomjs": "^1.9.18",
"protractor": "^4.0.8",
"protractor-jasmine2-screenshot-reporter": "^0.3.1"
"protractor-jasmine2-screenshot-reporter": "^0.3.1",
"stubby": "^4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks nice! I'll take a look on this library after.

@viniciusgama viniciusgama requested review from pedrro and removed request for wcalderipe January 26, 2017 21:58
@pedrro pedrro merged commit e76fa1e into master Jan 26, 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.

4 participants