-
Notifications
You must be signed in to change notification settings - Fork 210
Disputable voting app frontend #1194
Disputable voting app frontend #1194
Conversation
andy-hook
left a comment
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.
Looking good to me! 🚀 I added some points for clarification and an assortment of nits on the previous layout code now that we have it all together :)
apps/voting-disputable/app/src/components/VoteDetail/VoteInfoBoxes.js
Outdated
Show resolved
Hide resolved
| align-items: center; | ||
| `} | ||
| > | ||
| {challengeAmount} {collateralToken} |
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.
Just wondering, did we make a decision on whether to show the token icon? I know we discussed it previously, no worries if it's going to be a separate improvement!
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.
Yes I will add it in a separate PR :)
apps/voting-disputable/app/src/components/VoteDetail/DisputableActions.js
Outdated
Show resolved
Hide resolved
apps/voting-disputable/app/src/components/VoteDetail/DisputableActionStatus.js
Show resolved
Hide resolved
apps/voting-disputable/app/src/components/VoteDetail/VoteInfoBoxes.js
Outdated
Show resolved
Hide resolved
apps/voting-disputable/app/src/components/DetailedDescription.js
Outdated
Show resolved
Hide resolved
| Icon: IconClock, | ||
| }, | ||
| [VOTE_STATUS_CANCELLED]: { | ||
| background: String(theme.surfaceUnder), |
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.
We can remove these string conversions as they are already being performed in the template string when passed to the component.
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 passed down like this cause theme.surfaceUnder is an extended String object, and on this Tag component is seen as an object instead of a normal string and throughs an error
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.
Interesting! the template string that's being passed to the component at the moment should already be converting this so you shouldn't be seeing any errors by removing them 🤔 .
I did find an error in this file with an icon import though which might be causing the problems! We're importing IconClosed which should probably be IconClose
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.
Yes I modified it, thanks :)
apps/voting-disputable/app/src/components/DisputableStatusLabel.js
Outdated
Show resolved
Hide resolved
* Voting app frontend * Disputable voting components * Modify voting app files for disputable voting * Frontend data intoo manifest.json * imports and css change suggestions * Fix on status label * Add padding to info boxes
The first commit is just the voting app like it is on master.
The second one is the disputable changes from the PRs already approved.
And the other two is the new stuff. The changes on the voting app so it works here.
To try it out:
1- Include this line
[appIds['VotingDisputable']]: { script: '/script.js', start_url: '/index.html', },on enviroment.js on the client after this line2- Run the app
3- Run the client with:
ARAGON_APP_LOCATOR=0x705b5084c67966bb8e4640b28bab7a1e51e03d209d84e3a04d2a4f7415f93b34 npm run start:rinkeby