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

[Address Book V2] - Export address book#2284

Merged
katspaugh merged 5 commits into
address-book-v2from
feature/address-book-export
May 17, 2021
Merged

[Address Book V2] - Export address book#2284
katspaugh merged 5 commits into
address-book-v2from
feature/address-book-export

Conversation

@matextrem

Copy link
Copy Markdown
Contributor

What: This new feature allow users to export their Address book as CSV.

Why: As part of the new redesign of the Address Book. This PR closes #2235

How to test it:

  • Load a safe with some entries on the address book or Open a new one and add some entries.
  • Go to Address Book section.
  • Click on Export icon.

@matextrem matextrem added the Feature 👑 New functionality label May 11, 2021
@matextrem matextrem self-assigned this May 11, 2021
@github-actions

Copy link
Copy Markdown

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions

github-actions Bot commented May 11, 2021

Copy link
Copy Markdown

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
Copy Markdown

@github-actions

Copy link
Copy Markdown

@katspaugh katspaugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work, @matextrem!

Did you manage to simulate an error state?

Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
@katspaugh katspaugh changed the base branch from development to address-book-v2 May 12, 2021 09:59
@katspaugh

katspaugh commented May 12, 2021

Copy link
Copy Markdown
Contributor

Noticed a small bug:

  • if you click export
  • and then close the modal by clicking outside of the modal

it will show a success notification.

@lukasschor

Copy link
Copy Markdown
Contributor

CleanShot 2021-05-12 at 12 35 53@2x

Some feedback on the copy used in this PR:

The (only addresses and names) was confusing for me. What else should it be? Why do we need to mention this? I would just leave this out.

file with your XX Address Book entries - Shouldn't it say file with XX Address Book entries?

@tschubotz

Copy link
Copy Markdown
Contributor

Adding to what Lukas said, please call it addressbook everywhere for consistency, i.e. with out the whitespace in between. That how it's called in the sidebar currentl .

@katspaugh

katspaugh commented May 12, 2021

Copy link
Copy Markdown
Contributor

addressbook w/o a space doesn't seem to be the mainstream spelling (according to Google and Wikipedia).

@tschubotz

Copy link
Copy Markdown
Contributor

addressbook w/o a space doesn't seem to be the mainstream spelling (according to Google and Wikipedia).

I'm happy either way. I just think it needs to be consistent. I totally agree with you based on your links that the one with a space is more common. In English, words aren't connected together endlessly like in German.

Then let's use address book. But please adapt the sidebar label then too, please!

@alongoni alongoni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!!

Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
@matextrem matextrem requested review from alongoni and katspaugh May 12, 2021 16:40
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx
@matextrem

Copy link
Copy Markdown
Contributor Author

Great work, @matextrem!

Did you manage to simulate an error state?

I tried it by hardcoding the error state to see how the UI behaves

@nicosampler nicosampler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's implement the styles using styled-components instead of material 🙏🏾.
We should only use the styles from material when we are editing a component and we don't want to refactor the code, but as this is a completely new code, we should definitely go with styled.

Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx Outdated
Comment thread src/routes/safe/components/AddressBook/ExportEntriesModal/index.tsx
@matextrem matextrem requested a review from katspaugh May 14, 2021 21:34
@katspaugh

Copy link
Copy Markdown
Contributor

Looks and works great!

Just one last detail I would add: disable/add loader to the download button after clicking on it.

@katspaugh

Copy link
Copy Markdown
Contributor

I'll go ahead and merge this PR. Will make a separate ticket for the button state.

@katspaugh katspaugh merged commit 839c947 into address-book-v2 May 17, 2021
@katspaugh katspaugh deleted the feature/address-book-export branch May 17, 2021 14:55
@github-actions github-actions Bot locked and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Feature 👑 New functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Address Book v2] Export the address book

6 participants