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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds survey app #126

Merged
merged 3 commits into from Aug 1, 2019

Conversation

@ajsantander
Copy link
Contributor

commented Aug 1, 2019

馃弫 Ready for Review 馃弫
Fix #104

@ajsantander ajsantander requested a review from facuspagnuolo Aug 1, 2019

@luisivan luisivan referenced this pull request Aug 1, 2019
17 of 19 tasks complete
@sohkai

sohkai approved these changes Aug 1, 2019

Copy link
Member

left a comment

馃憤 Couple of small things, but LGTM!

@@ -35,6 +35,7 @@
"@aragon/apps-vault": "^4.1.0",
"@aragon/apps-voting": "^2.1.0",
"@aragon/apps-finance": "^3.0.0",
"@aragon/apps-survey": "^1.0.0",

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 1, 2019

Member

I don't think these other templates need the survey dependency?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 1, 2019

Member

I used to have problems with this as well, for some reason the templates using the BaseTemplate were asking for all the apps being used by the base :/
Will merge and polish it if possible

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 1, 2019

Author Contributor

I was experiencing the problem of the templates not compiling if the Survey app was not added. Same thing happened when I was adding the Payroll app.


function _installSurveyApp(Kernel _dao, MiniMeToken _token, uint64 _minParticipationPct, uint64 _surveyTime) internal returns (Survey) {
Survey survey = Survey(_installNonDefaultApp(_dao, SURVEY_APP_ID));
survey.initialize(_token, _minParticipationPct, _surveyTime);

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 1, 2019

Member

Once #125 is merged, we should use the optimized version too :).

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 1, 2019

Member

Merging both.. will provide a third PR to update this :)

@facuspagnuolo
Copy link
Member

left a comment

LGTM

@facuspagnuolo facuspagnuolo merged commit c214bfa into master Aug 1, 2019

4 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@delete-merged-branch delete-merged-branch bot deleted the add_survey_app branch Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.