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.

Sign up for GitHub

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

Jump to bottom

feat: vote modal with reason #427

Merged
merged 43 commits into from
Jul 15, 2024
Merged

feat: vote modal with reason #427

merged 43 commits into from
Jul 15, 2024

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Jun 23, 2024 •
edited
Loading

Summary

Closes: #386

This PR moves the vote UI inside a vote modal, similar to v1, and allow attaching a reason to a vote.

Features

  • new vote modal: voting for a choice will open a Vote modal, where user can see a summary of its vote and voting power, before submitting the vote. This step will add an extra guard layer, where we can prevent users without sufficient voting power from voting.
  • vote with reason: a new reason field has been added in the vote modal, where the voter can attach a reason to his vote.
  • refactoring the getVotingPower logic into a store and composable, similar to how we handle Space, enabling caching and avoid code duplicate

How to test

  1. Try to vote from inside a proposal page, or a proposal list (space proposals/home feed)
  2. It should open the vote modal if logged in, else will open the account modal
  3. The vote modal should show your selected choice, as well as your voting power for the current proposal
  4. The CONFIRM button should be disabled if you don't have sufficient voting power to vote
  5. You can attach an optional reason to your vote
  6. Vote is limited to 1000 chars, and should disable the form on error
  7. Clicking on the CONFIRM should vote like before if no reason, and will add a metadataUri (onchain) or reason (offchain) field if set
  8. It should show a warning message and disable the submit button when the vp fetching has failed or on insufficient vp
  9. On the create proposal page and vote modal, it should show a RETRY button to refetch the voting power, in case of fetching error
  10. Voting power should be cached, you should not see the loading indicator when getting the same vp again on same space and block, and should show the vp immediately.
  11. Voting power should refresh on wallet change/logout

TODO

  • Confirm reason works offchain
  • Confirm reason works on EVM testnet (waiting for thegraph fix on the sepolia graph)
  • Confirm reason works on EVM mainnet
  • Confirm reason works on Starknet testnet
  • Confirm reason works on Starknet mainnet (signature error, waiting for remove verifyingContract from domain #464 as potential fix)
  • Fix/Update tests

Future

@wa0x6e wa0x6e requested review from Sekhmet and bonustrack and removed request for Sekhmet and bonustrack June 24, 2024 13:10
@bonustrack bonustrack requested review from Sekhmet and removed request for Sekhmet June 24, 2024 13:15
@wa0x6e wa0x6e marked this pull request as draft June 24, 2024 21:39
@wa0x6e wa0x6e changed the title feat(ui): share vote modal feat(ui): vote modal Jun 25, 2024
@wa0x6e wa0x6e changed the title feat(ui): vote modal feat(ui): vote modal with reason Jun 25, 2024
@wa0x6e wa0x6e marked this pull request as ready for review June 25, 2024 07:10
@wa0x6e
Copy link
Contributor Author

wa0x6e commented Jun 27, 2024

@Sekhmet I got this working on offchain, but will need some help indexing the vote for evm, and also maybe starknet

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Jun 27, 2024

@bonustrack The vote reason is now working for offchain, and you can start testing the UI and UX.

Main testing area will be the UI/UX on the modal when the user don't have enough voting power, or failing to fetch voting power.

bonustrack
bonustrack reviewed Jun 27, 2024
View reviewed changes
Copy link
Member

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

Flow works well for me just added a small suggestion

apps/ui/src/components/MessageVotingPower.vue Outdated Show resolved Hide resolved
@wa0x6e wa0x6e changed the title feat(ui): vote modal with reason feat: vote modal with reason Jun 28, 2024
Sekhmet
Sekhmet reviewed Jun 28, 2024
View reviewed changes
apps/ui/src/components/Ui/Button.vue
@@ -27,7 +27,7 @@ withDefaults(
</button>
</template>

<style scoped lang="scss">
Copy link
Member

Choose a reason for hiding this comment

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

Why is it no longer scoped? That's generally not good because it will leak styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the UILoading icon can not be styled when inside the button with the scope

Copy link
Member

Choose a reason for hiding this comment

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

Can you link to where do you use it like this (couldn't find it in the diff) so I can take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to fix the loading icon, on the vote modal CONFIRM button, when pressed, and while waiting for wallet confirmation

bonustrack
bonustrack approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

tACK

@Sekhmet
Copy link
Member

Sekhmet commented Jul 11, 2024

@wa0x6e typechecks are failing there

@wa0x6e wa0x6e mentioned this pull request Jul 12, 2024
Merged
2 tasks
Sekhmet
Sekhmet reviewed Jul 12, 2024
View reviewed changes
Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

Other than that it looks good to me!

packages/sx.js/src/clients/offchain/ethereum-sig/index.ts Show resolved Hide resolved
apps/ui/src/types.ts Outdated Show resolved Hide resolved
Sekhmet
Sekhmet approved these changes Jul 15, 2024
View reviewed changes
Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

tACK

@Sekhmet Sekhmet merged commit ad6fc9e into master Jul 15, 2024
3 checks passed
@Sekhmet Sekhmet deleted the feat-vote-modal-with-reason branch July 15, 2024 10:05
@Sekhmet Sekhmet mentioned this pull request Jul 15, 2024
Merged
Sekhmet added a commit that referenced this pull request Jul 16, 2024
@Sekhmet
Revert "feat: vote modal with reason (#427)"
6aa62f5 
This reverts commit ad6fc9e.
Sekhmet added a commit that referenced this pull request Jul 16, 2024
@Sekhmet
Revert "feat: vote modal with reason (#427)" (#495)
0e66d32 
* Revert "feat: vote modal with reason (#427)"

This reverts commit ad6fc9e.

* Revert "chore: update help desk link format after merge (#484)"

This reverts commit 78044d3.
@wa0x6e wa0x6e mentioned this pull request Jul 20, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonustrack bonustrack bonustrack approved these changes

@Sekhmet Sekhmet Sekhmet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: vote modals for reason, progress and share
3 participants
@wa0x6e @Sekhmet @bonustrack