Closed tomhirst closed 2 years ago
Does this work okay if there is an ampersand or > or < than in the title / description? I'm wondering if this is going to be double escaping things.
Does this work okay if there is an ampersand or > or < than in the title / description? I'm wondering if this is going to be double escaping things.
It does now. Good catch, ser.
Wearing my paranoid hat here, since an escaping attack on the voting UI would be critical.
I'm concerned that since the dangerouslySetInnerHTML
's are in a separate file from the actual escaping, that future changes could inadvertently mess things up.
In getProposalContent
can we add a comment that the output of this function must be escaped and safe for use as raw HTML in the UI? And could we change the names of in output to append either "safe", "sanitized" or "clean", so that when writing the front end code you know that it's been checked elsewhere.
Ideally we'd have unit tests checking that no scripts could make it through, but we can skip this since we don't have front end browser tests.
Wearing my paranoid hat here, since an escaping attack on the voting UI would be critical.
I'm concerned that since the
dangerouslySetInnerHTML
's are in a separate file from the actual escaping, that future changes could inadvertently mess things up.In
getProposalContent
can we add a comment that the output of this function must be escaped and safe for use as raw HTML in the UI? And could we change the names of in output to append either "safe", "sanitized" or "clean", so that when writing the front end code you know that it's been checked elsewhere.Ideally we'd have unit tests checking that no scripts could make it through, but we can skip this since we don't have front end browser tests.
Good points. Suggestions added.
Updates from recent feedback post-demo:
Will the active tag update to executed when the proposal is completed? It might be good to show a proposal history on the proposal pages.
Could add a back button back to all proposals. I'm used to seeing these on governance UIs.
Could visualise the vote allocation with some kind of bar chart/line diagram like so:
I'm unable to click on the contract address in the description box.
I'm not really sure what Arguments and Argument types are. Tbh the whole "Description" box doesn't make much sense to me.
Same for the details box. We could look at simplifying this to make it more understandable.
Small UI thing but I struggle to read the text on these disabled buttons. Could up the contrast in the button text so its readable enough whilst still looking disabled.
Great feedback, thanks @matt-bullock.
From a technical point of view, everything Compound has on screen, we now have. It doesn't look as good as what they have though. Should we invest some time rejigging things in Figma to attain a better layout? Let me know what you think @matt-bullock and @micahalcorn.
Video overload 🌊
P2
P3
@tomhirst @micahalcorn I'd be happy to work on the design for this. As we have a new brand style/ look and feel which we will soon be introducing, would it would make sense for me to start re-designing it in this style?
@tomhirst @micahalcorn I'd be happy to work on the design for this. As we have a new brand style/ look and feel which we will soon be introducing, would it would make sense for me to start re-designing it in this style?
Nice! Good point, though. Perhaps my v1 will be good enough from a design perspective until we roll out the wider re-style across Origin's products? However, if you can think of any quick layout wins in the current style, let me know and I'll implement them.
I think this is fine for v1. I look forward to giving it a proper redesign when the time comes. A couple of quick wins that would help in the meantime:
Being able to click the entire area of the proposal to go into it, rather than specific text links. Hopefully this makes sense:
It would be good to show the for and against percentage bars even if one side has no votes (it would just be the empty grey bar). Like you have here:
Small alignment thing. You can line the text up here so it's all aligned
For the progress bar in the details section it would help to have the grey bar in the background so show where the current progress sits in relations to being 100% complete. Like you have the grey background in the for and against vote bars.
Video overload 🌊
P2
P3
Thanks for this @micahalcorn - I've worked through them all and have made some additions to the staging environment.
A couple of points:
Improve proposal states: We can grab a future start/end block timestamp estimate from the Etherscan API, though the free tier rate limits us to 5 calls per second. This works if a user is viewing a single proposal, but if we want to display these timestamp estimates in the proposal index view, we'd have to upgrade to a paid plan
Avoid truncating addresses: I've untruncated the proposer addresses in the proposal single view as there's room for it to fall onto the next line. However, in the tables (for/against and contract actions) we don't have the room to show the full addresses. I think if we link out to Etherscan (and thus the full address) shortening is OK. If we expand the wrapper to fill the full screen, we'll have to consider the wider design of the app and I think that's a bridge too far for this PR.
@micahalcorn Here are a few screenshots from my local testing to add a little colour to the different proposal states:
Proposal index
Pending proposal
Active proposal
Defeated proposal
Succeeded proposal
Note: In a non-local environment, proposals that succeed will eventually be queued and executed. These events will be picked up by the listener and will further populate the proposal history panel with timestamps and transactions.
Note: In a non-local environment, proposals that succeed will eventually be queued and executed. These events will be picked up by the listener and will further populate the proposal history panel with timestamps and transactions.
Amazing, thanks 👏 I'd suggest that we go ahead and proceed. We can continue to test with the new staging contracts and log potential improvements while we move forward as-is with production.
I gave the "register to vote" CTA some thought overnight and devised this solution. I've re-used an existing CTA pattern to display the section to users with veOGV-positive wallets that haven't self-delegated. This will appear on the home page (last 5 proposals) and the proposals index page.
LMK if you think anything further is needed @matt-bullock and if you'd like to revise the copy at all @micahalcorn.
I've included the CTA on the single proposal view too, thanks for the recommendation @matt-bullock
This PR gets the governance part of the dApp up to a usable v1 level:
Once tested as functional on staging + merged, we can look at adding improvements (with product + design input) in smaller PRs.
Don't merge prior to testing on Rinkeby
Some screenshots from my local environment: