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

feat: Add StepperFactory and new tx layout #2040

Merged
merged 7 commits into from
May 30, 2023
Merged

Conversation

usame-algan
Copy link
Member

@usame-algan usame-algan commented May 25, 2023

What it solves

Part of #2021

How this PR fixes it

  • Adds new global context ModalProvider for orchestrating modals in the app
  • Adds new folder structure TxFlow
  • Adds NewTxMenu, ReplaceTxMenu, RejectTx, TokenTransfer to the provider
  • Adds TxLayout component
  • Adds StepperFactory and uses it for TokenTransferFlow

ToDos

  • Close Modal when navigating in the sidebar
  • Resolve TODOs

How to test it

  1. Open the app
  2. Press on New Transaction in the sidebar
  3. Observe the NewTxMenu opening as a fullscreen modal
  4. Press on Send Tokens
  5. Observe the new TokenTransferFlow opening as a fullscreen modal
  6. Navigate to /queue
  7. Replace an existing transaction
  8. Observe the ReplaceTxMenu opening as a fullscreen modal
  9. Press on either Send Tokens or Reject Transaction
  10. Observe TokenTransferFlow or RejectTx opening as a fullscreen modal

Screenshots

Screenshot 2023-05-26 at 11 51 16 Screenshot 2023-05-26 at 11 51 24 Screenshot 2023-05-26 at 11 51 35 Screenshot 2023-05-26 at 11 51 46

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@github-actions
Copy link

github-actions bot commented May 25, 2023

Branch preview

✅ Deploy successful!

https://add_layout--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented May 25, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@usame-algan usame-algan requested review from katspaugh and iamacook May 26, 2023 09:51
@usame-algan usame-algan marked this pull request as ready for review May 26, 2023 09:54
@usame-algan usame-algan changed the title WIP: Add StepperFactory and new tx layout feat: Add StepperFactory and new tx layout May 26, 2023
Comment on lines 12 to 15
import TokenTransferFlow from '@/components/TxFlow/TokenTransfer/TokenTransferFlow'
import RejectTx from '@/components/TxFlow/RejectTx'
import NewTxMenu from '@/components/TxFlow/NewTx'
import ReplaceTxMenu from '@/components/TxFlow/ReplaceTx'
Copy link
Member

Choose a reason for hiding this comment

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

This part can be extracted into another module. Which could be imported and inserted here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a new file and exported all components from there. Is this what you have envisioned?

Comment on lines +18 to +30
export enum ModalType {
SendTokens = 'sendTokens',
RejectTx = 'rejectTx',
ReplaceTx = 'replaceTx',
NewTx = 'newTx',
}

const ModalTypes = {
[ModalType.SendTokens]: TokenTransferFlow,
[ModalType.RejectTx]: RejectTx,
[ModalType.ReplaceTx]: ReplaceTxMenu,
[ModalType.NewTx]: NewTxMenu,
}
Copy link
Member

@katspaugh katspaugh May 26, 2023

Choose a reason for hiding this comment

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

I've just realized it's probably easier to just let modal-opening buttons pass TokenTransferFlow directly instead of a string. That would make the dependency explicit.

So instead of

onClick={() => setVisibleModal({ type: ModalType.SendTokens, props: {} })}

they would do

onClick={() => setVisibleModal(<TokenTransferFlow nonce={nonce} />)}

Not 100% convinced tho.

Copy link
Member

Choose a reason for hiding this comment

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

The implementation we opted for aimed to narrow the possible components we want to show in the modal. It comes at the cost of maintaining the map but setting a component in the dispatcher would make harder to reinforce the types.
We are also not sold on storing the component as state.

Copy link
Member

@katspaugh katspaugh May 30, 2023

Choose a reason for hiding this comment

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

You can still limit the type of components setVisibleModal accepts.
Something like T extends TxFlowComponent, so that it accepts any component whose signature satisfies TxFlowComponent.

type: SendTxType.multiSig,
}

const TokenTransferFlow = ({ txNonce }: { txNonce?: number }) => {
Copy link
Member

Choose a reason for hiding this comment

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

The naming is a bit inconsistent, this is the only transaction type named with "Flow" at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

We were also thinking of naming RejectTx to RejectTxFlow but it only has one step and we didn't use the Stepper there so we left it as RejectTx.

@usame-algan usame-algan requested a review from katspaugh May 30, 2023 09:25
@usame-algan usame-algan merged commit e2c9972 into epic-tx-flow May 30, 2023
@usame-algan usame-algan deleted the add-layout branch May 30, 2023 12:01
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants