Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Add EIP-3770 settings #2888

Merged
merged 5 commits into from
Oct 26, 2021
Merged

Add EIP-3770 settings #2888

merged 5 commits into from
Oct 26, 2021

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Oct 25, 2021

What it solves

Resolves #2843

How this PR fixes it

  • A new reducer has been added under the appearance key to store the new settings.
  • A new settings subsection 'Appearance' has been added. It currently has EIP-3770 settings regarding showing/copying chain shortName prefixes.
  • A checkbox has been added to the QR modals to allow for copying with/without shortName. It defaults to the value set in settings and does not update the store.

Note: the settings do not currently affect any of the UI except the QR modal.

How to test it

Ensure that changes in the 'Appearance' settings save locally.
Open QR modal and check that the value reflects that saved.
Scan the QR code with/without the checkbox checked to see if shortName is prepended.

Screenshots

image
image

@iamacook
Copy link
Member Author

@DiogoSoaress, you can find the selectors here.

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Oct 25, 2021

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

@github-actions
Copy link

@coveralls
Copy link

coveralls commented Oct 25, 2021

Pull Request Test Coverage Report for Build 1384599357

  • 6 of 29 (20.69%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 31.316%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/logic/appearance/reducer/appearance.ts 2 4 50.0%
src/logic/appearance/selectors/index.ts 0 2 0.0%
src/components/App/ReceiveModal.tsx 0 4 0.0%
src/routes/safe/components/Settings/index.tsx 0 4 0.0%
src/routes/safe/components/Settings/Appearance/index.tsx 0 11 0.0%
Files with Coverage Reduction New Missed Lines %
src/components/App/ReceiveModal.tsx 1 0%
Totals Coverage Status
Change from base Build 1384262732: -0.04%
Covered Lines: 2919
Relevant Lines: 8336

💛 - Coveralls

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.

Alles top!

<Paragraph>
{showShortName && <strong>{shortName}:</strong>}
{safeAddress}
</Paragraph>
Copy link
Member

Choose a reason for hiding this comment

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

Once we update EthHashInfo, we can use it here.

Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

Very clean implementation!

Noticed that this breadcrumb is incomplete
Screen Shot 2021-10-25 at 16 51 57

@iamacook
Copy link
Member Author

Very clean implementation!

Noticed that this breadcrumb is incomplete Screen Shot 2021-10-25 at 16 51 57

Good catch! I've added it :)

Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

@francovenica
Copy link
Contributor

The main functionality works fine:
The checkboxes change the local storage
The preview address works fine, adding/removing the prefix
The QR code for the safe works fine, changing when it adds the prefix
The settings are saved across safes, as the message implies.

Issue:
1 - Small issue that I don't see happening in prod so I assume it was introduced here.
By being in any subpage of the settings (can be appearance or owners or what ever) if you reload the page the setting menu closes and the breadcrumb of the page breaks.
The expected behavior is that the setting menu remains open and the section of the settings you are is highlighted

Gif:
10-26-2021_x(3214)

@iamacook
Copy link
Member Author

iamacook commented Oct 26, 2021

The main functionality works fine: The checkboxes change the local storage The preview address works fine, adding/removing the prefix The QR code for the safe works fine, changing when it adds the prefix The settings are saved across safes, as the message implies.

Issue: 1 - Small issue that I don't see happening in prod so I assume it was introduced here. By being in any subpage of the settings (can be appearance or owners or what ever) if you reload the page the setting menu closes and the breadcrumb of the page breaks. The expected behavior is that the setting menu remains open and the section of the settings you are is highlighted

Gif: 10-26-2021_x(3214)

Good to hear that all works as expected. I will merge this and have created an issue to fix the bug here.

Thanks for testing this so thoroughly 👍

@iamacook iamacook merged commit 4db62b3 into dev Oct 26, 2021
@iamacook iamacook deleted the eip3770-settings branch October 26, 2021 07:49
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2021
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 a new Settings screen for EIP-3770 preferences
5 participants