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: redesign Load Safe #1405

Merged
merged 15 commits into from
Jan 2, 2023
Merged

feat: redesign Load Safe #1405

merged 15 commits into from
Jan 2, 2023

Conversation

usame-algan
Copy link
Member

@usame-algan usame-algan commented Dec 20, 2022

What it solves

Resolves #964

How this PR fixes it

  • Existing components from src/components/load-safe were copied into the new structure within src/components/new-safe/load with minor layout adjustments
  • The first and second step of the old flow are combined
  • Removes the old routes /open and /load and their pages
  • Uses the new routes /new-safe/create and /new-safe/load everywhere
  • Restructures the new-safe directory

ToDos

  • Move OwnerRow into the parent directory
  • Remove old load safe components inside src/components/load-safe
  • Find a solution for cut-off safe addresses in the new design

How to test it

  1. Open the Safe
  2. Navigate to the welcome page
  3. Click on "Add existing Safe"
  4. Observe the new design
  5. Navigate to a specific safe
  6. Open the safe list sidebar and click on "Add Safe"
  7. Observe being navigated to the load safe flow and the safe address being pre-filled

Screenshots

Screenshot 2022-12-20 at 14 59 24

Screenshot 2022-12-21 at 11 49 22

Screenshot 2022-12-20 at 15 05 38

@github-actions
Copy link

github-actions bot commented Dec 20, 2022

Branch preview

✅ Deploy successful!

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

@github-actions
Copy link

github-actions bot commented Dec 20, 2022

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 marked this pull request as ready for review December 20, 2022 14:47
@katspaugh katspaugh changed the title Redesign Load Safe feat: redesign Load Safe Dec 20, 2022
@katspaugh
Copy link
Member

Screenshot 2022-12-20 at 15 53 50

☝️ these inputs look read-only until focused.

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

The code looks good, and in my testing, it also works fine.

I don't like calling components Step0, Step1 etc, but it's the same in the creation...

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@katspaugh
Copy link
Member

Screenshot 2022-12-22 at 09 00 55

☝️ can we stretch the address field to the full width?

@katspaugh
Copy link
Member

I've stretched the address field like so:

Screenshot 2022-12-27 at 09 34 05

Screenshot 2022-12-27 at 09 34 14

@francovenica
Copy link
Contributor

Issue:

When using and ENS name, after the name is translated to a valid address, is expected to also be autofilled in the "Name" input.
The current result is that the generic random name is still used to fill the input
Try "goerlisafe.eth" for testing

image

@francovenica
Copy link
Contributor

Correction. I've checked and the ENS autofill name is broken in prod as well. It would be great if that can be fixed here, but is not a blocker to pass this ticket.
Let me know if that can be fixed here or if I should open a new ticket

@usame-algan
Copy link
Member Author

usame-algan commented Jan 2, 2023

Correction. I've checked and the ENS autofill name is broken in prod as well. It would be great if that can be fixed here, but is not a blocker to pass this ticket. Let me know if that can be fixed here or if I should open a new ticket

For me it resolves the ENS and autofills it as a placeholder to the name input. I think this is the intended behaviour as it was changed to be a placeholder in #353

The reason why goerlisafe.eth is not resolving is because it needs to be set as a reverse record entry to the safe address

@github-actions
Copy link

github-actions bot commented Jan 2, 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

@francovenica
Copy link
Contributor

I didn't noticed that the name wasn't properly set. Now it works fine

@usame-algan usame-algan merged commit 7b2cc49 into dev Jan 2, 2023
@usame-algan usame-algan deleted the add-safe branch January 2, 2023 15:52
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 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.

[Add Safe] Implement redesign
4 participants