-
Notifications
You must be signed in to change notification settings - Fork 7
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
[ENG-4]
Ensure Voting Button disappears after voting
#2688
base: develop
Are you sure you want to change the base?
Conversation
Deploying decent-interface with
|
Latest commit: |
3da1bed
|
Status: | ✅ Deploy successful! |
Preview URL: | https://be77f930.decent-interface.pages.dev |
Branch Preview URL: | https://eng-4-button-disappears-afte.decent-interface.pages.dev |
proposal.state === FractalProposalState.TIMELOCKED; | ||
|
||
return isSnapshotProposal || isAzoriusProposal || isOtherProposalStates; | ||
}, [snapshotProposal, canVote, hasVoted, isActiveProposal, proposal.state]); |
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.
lool I see
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.
Oh, probably memoizing would solve that flickering issue
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'll memoize anyway just a good idea in general but I didn't notice any flickering. but don't doubt it
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 see couple logical issues - could you verify my assumptions regarding suitability of canVote
and use only that one for isAzoriusProposal
?
Also, I feel like names of variables are little misleading. I'd suggest renaming:
isSnapshotProposal
->hasActionOnSnapshotProposal
isAzoriusProposal
->hasActionOnAzoriusProposal
proposal.state === FractalProposalState.TIMELOCKABLE || | ||
proposal.state === FractalProposalState.TIMELOCKED; | ||
const showActionButton = useMemo(() => { | ||
const isSnapshotProposal = snapshotProposal && canVote && isActiveProposal && !hasVoted; |
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.
On snapshot proposal you can re-do your vote - that was intentional to keep the button, cause comparing to Azorius / Multisig - you can not re-cast your vote.
Except for ERC-721 - there's a case when your voting power changed during the vote, and then you can vote once more (even for different option!) with the remaining tokens (aka tokens that weren't used during last time you've casted your vote).
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.
huh? lol Okay gonna need to have to write this all out on what states and what we want it to look like @nicolaus-sherrill
proposal.state === FractalProposalState.TIMELOCKED; | ||
const showActionButton = useMemo(() => { | ||
const isSnapshotProposal = snapshotProposal && canVote && isActiveProposal && !hasVoted; | ||
const isAzoriusProposal = canVote && isActiveProposal && !hasVoted; |
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.
So yeah - with ERC-721 it's a little more tricky
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 probably should rely just on canVote && isActiveProposal
. I'd expect that canVote
returns false
for ERC-20 when you already voted, but true
for ERC-721 if you have remaningTokensIds.length > 0
.
proposal.state === FractalProposalState.TIMELOCKED; | ||
|
||
return isSnapshotProposal || isAzoriusProposal || isOtherProposalStates; | ||
}, [snapshotProposal, canVote, hasVoted, isActiveProposal, proposal.state]); |
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.
Oh, probably memoizing would solve that flickering issue
Also I really feel like we've had a discussion that it would be preferable for button to stay but keep it disabled while voting is on... |
Closes ENG-4