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

refactor: replace Stepper for MUI's List #1467

Merged
merged 6 commits into from
Jan 18, 2023
Merged

refactor: replace Stepper for MUI's List #1467

merged 6 commits into from
Jan 18, 2023

Conversation

DiogoSoaress
Copy link
Member

@DiogoSoaress DiogoSoaress commented Dec 28, 2022 •
edited by liliya-soroka
Loading

What it solves

Resolves #1218

How this PR fixes it

Replaces TxSigners logic around MUI's Stepper by MUI's List in alignment with MsgSigners component.

How to test it

The TxSigners stepper should work as before for all types of queued and historic transactions.
Icons colors should remain unaltered.

Screenshots

Screenshot 2022-12-28 at 17 11 03
Screenshot 2022-12-28 at 17 11 14

Expected behaviour

If the connected account can execute the queued tx it will show the checkmark icon. In the case you describe, the connected account can execute because if it signs, it will satisfy the threshold

@github-actions
Copy link

github-actions bot commented Dec 28, 2022 •
edited
Loading

Branch preview

✅ Deploy successful!

https://refactor_tx_signers_stepper--webcore.review-web-core.5afe.dev

@DiogoSoaress DiogoSoaress changed the title refactor: replace Stepper for MUI List. Prefer custom svg over MUI icons refactor: replace Stepper for MUI's List Dec 28, 2022
@github-actions
Copy link

github-actions bot commented Dec 28, 2022 •
edited
Loading

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

@DiogoSoaress
Copy link
Member Author

@liliiaorlenko In order to completely replace MUI's icons in this component by our custom ones, we will need:

🙏

iamacook
iamacook reviewed Jan 5, 2023
View reviewed changes
src/components/transactions/TxSigners/index.tsx Outdated
@@ -19,18 +22,21 @@ import { isCancellationTxInfo, isExecutable, isMultisigDetailedExecutionInfo } f
import EthHashInfo from '@/components/common/EthHashInfo'

import css from './styles.module.css'
import txSignersCss from '@/components/transactions/TxSigners/styles.module.css'
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 already imported as css.

Copy link
Member

Choose a reason for hiding this comment

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

We should also reuse this in MsgSigners.

src/components/transactions/TxSigners/index.tsx Outdated
import useSafeInfo from '@/hooks/useSafeInfo'
import palette from '@/styles/colors'
Copy link
Member

Choose a reason for hiding this comment

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

This does not take dark mode into account:

image

@katspaugh
Copy link
Member

Please update the branch.

@liliya-soroka
Copy link
Member

liliya-soroka commented Jan 16, 2023 •
edited
Loading

  1. @DiogoSoaress , the red circle is changed to the green one during the signing process and comes back to the red one after the signing is done.
    Should we display the right circle state only when the signatures are updated?

image

@liliya-soroka
Copy link
Member

2nd issue :

  1. There are 2 owners in the MM ( owner A and owner B) and 2 of N safe
  2. Create tx using owner A
  3. reject created tx as an owner A
  4. switch to the owner B
  5. Confirm first tx without execution as owner B
  6. open rejection tx
    Current result: both tx has "all confirmations are received" circle . But only the fully confirmed tx should have this icon

image

@DiogoSoaress
Copy link
Member Author

DiogoSoaress commented Jan 16, 2023 •
edited
Loading

Ty for QAing @liliya-soroka

  1. I am trying to reproduce it. Can you perhaps provide a short video? Is it only in a 3/N setup?
    edit: I already got it

  2. That is expected. The logic we have in PROD is maintained in this PR
    If the connected account can execute the queued tx it will show the checkmark icon. In the case you describe, the connected account can execute because if it signs, it will satisfy the threshold

@github-actions
Copy link

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

@DiogoSoaress
Copy link
Member Author

@liliya-soroka please recheck. Point 1 should be fixed

@DiogoSoaress DiogoSoaress merged commit 7e6aa6c into dev Jan 18, 2023
@DiogoSoaress DiogoSoaress deleted the refactor-tx-signers-stepper branch January 18, 2023 12:17
@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iamacook iamacook iamacook 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.

Refactor TxSigners to match MsgSigners
4 participants
@DiogoSoaress @katspaugh @liliya-soroka @iamacook