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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2 tx flow, first remove votes and undelegate then delegate #186

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

Tbaut
Copy link
Contributor

@Tbaut Tbaut commented Oct 10, 2024

closes #185
closes #167

image


Submission checklist:

Layout

  • Change inspected in the mobile web ui
  • Change inspected in dark mode

@Tbaut Tbaut requested a review from wirednkod as a code owner October 10, 2024 12:54
@Tbaut Tbaut changed the base branch from main to tbaut-refactor-delegationCard October 10, 2024 12:54
Copy link

cloudflare-workers-and-pages bot commented Oct 10, 2024

Deploying delegit with  Cloudflare Pages  Cloudflare Pages

Latest commit: d4e684d
Status: ✅  Deploy successful!
Preview URL: https://144b8ca9.delegit.pages.dev
Branch Preview URL: https://tbaut-2-tx.delegit.pages.dev

View logs

Base automatically changed from tbaut-refactor-delegationCard to main October 10, 2024 15:38
title: string
message: string
title?: string
message: string | React.ReactNode
Copy link
Member

Choose a reason for hiding this comment

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

this is kinda scary.. We should probably split in 2 different optional variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah! this is pretty common and doesn't change anything in the code below. Having 2 variables makes it uglier, and doesn't change anything! For instance that's what we have in the ContentReveal and what many common libraries have too!

src/components/LocksCard.tsx Outdated Show resolved Hide resolved
src/components/LocksCard.tsx Outdated Show resolved Hide resolved
@@ -147,9 +147,9 @@ export const MultiTransactionDialog = ({
<DialogDescription>
<div className="my-4">
{step === 1 &&
'The delegation process is in 2 parts. First, please sign a transaction to remove current votes and delegations.'}
'The delegation process is in 2 parts. First, please sign a transaction to remove previous votes or delegations.'}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'The delegation process is in 2 parts. First, please sign a transaction to remove previous votes or delegations.'}
'Please sign a transaction to clear any previous votes or delegations before proceeding with the delegation process.'}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find this long phrase that we had actually pretty bad, nobody reads such long things, it needs to be more actionable IMHO. What do you think about:

There will be 2 transactions:
1. Clear previous votes & delegations
2. Delegate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edit, scratch that, I'll make a new proposal. We really don't need a long phrase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for something straight forward. No need to "Please" and what not, they are asking for it, not delegit!

  • First, let's clear previous votes or delegations.
  • Now, we are ready to delegate.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds better.

@Tbaut Tbaut requested a review from wirednkod October 10, 2024 17:07
@Tbaut
Copy link
Contributor Author

Tbaut commented Oct 12, 2024

ping @wirednkod for another review please 🙏

@wirednkod wirednkod merged commit 3bef517 into main Oct 14, 2024
3 checks passed
@wirednkod wirednkod deleted the tbaut-2-tx branch October 14, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants