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

Feature/daef 300 exporting wallet to file #426

Merged
merged 26 commits into from
Aug 23, 2017

Conversation

DominikGuzei
Copy link
Member

@DominikGuzei DominikGuzei commented Aug 18, 2017

This PR adds the "export to file" feature on wallet settings, which currently doesn't work correctly on the backend (it exports all wallets) but is mostly done on the frontend. I think we should wait with implementing acceptance tests for the feature until we have a working backend. If the wallet has a spending password it is also asked for but currently not passed to the backend since that's not supported yet.

wallet-export-to-file-feature

Apart from the feature, the following smaller fixes and improvements have been made:

  • Made storybook work with happypack (performance improvements)
  • Added global translations for spending password (but not yet fully refactored all other usages!)
  • Moved the export dialogs etc. into dedicated folders and simplified their structure.
  • Improved flow typing within the component / api / action, which can be used as a guideline for refactorings and future features.

TODO:

  • Checkout / fix storybook problem that nikola had
  • Add missing Japanese translations
global.spendingPasswordLabel: !!!Spending Password
global.spendingPasswordPlaceholder: !!!Password
wallet.exportToFile.dialog.headline: !!!Export Wallet
wallet.exportToFile.dialog.introduction: !!!You are exporting <strong>{walletName}</strong> to a file.
wallet.exportToFile.dialog.submit.label: !!!Export

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Great work @DominikGuzei!
New feature works great!
I have only fixed few minor eslint warnings and ran npm run manage-translations to populate missing translation keys.

I still have some questions/issues that I faced:

  1. Why have you removed paper-wallet-export-dialog? Is this export-to-file a replacement for the paper one?
  2. We are missing Japanese translations:
global.spendingPasswordLabel: !!!Spending Password
global.spendingPasswordPlaceholder: !!!Password
wallet.exportToFile.dialog.headline: !!!Export Wallet
wallet.exportToFile.dialog.introduction: !!!You are exporting <strong>{walletName}</strong> to a file.
wallet.exportToFile.dialog.submit.label: !!!Export
  1. I was unable to run storybook - this is the error I get when running npm run storybook:
    screen shot 2017-08-19 at 13 31 57

@darko-mijic
Copy link
Contributor

darko-mijic commented Aug 21, 2017

Why have you removed paper-wallet-export-dialog? Is this export-to-file a replacement for the paper one?

No. We need paper wallets but we are still waiting for the endpoint. We need to bring this back.

Translations are in the works.

@DominikGuzei
Copy link
Member Author

@nikolaglumac thanks for reviewing, im re-installing my npm deps to see if i can reproduce your storybook problem!

@DominikGuzei
Copy link
Member Author

@nikolaglumac @darko-mijic i think we don't need to bring back the WalletExportDialog in my PR because this dialog was just the "main entry point" for exports in general … since we don't expose paper wallet exports yet we don't need to show that option in the ui. All the specific paper wallet dialogs are still available in the sub-folder paper-wallet-export-dialogs

@DominikGuzei
Copy link
Member Author

@nikolaglumac i found the issue with broken storybook (im referencing electron in the export dialog container to open the dialog via remote, but that import fails in storybook because it's not a electron-renderer environment), trying to fix this now …

@DominikGuzei
Copy link
Member Author

@nikolaglumac all issues apart from translations are fixed now!

@nikolaglumac
Copy link
Contributor

Great @DominikGuzei - will review again in the evening ;)

@nikolaglumac
Copy link
Contributor

@DominikGuzei I have checked your update and fixed some minor theming/styling issues on Settings screen as well as loading inline-svgs in storybook.
I still have issues running storybook :(
Staking components that we don't use in the app are to blame:
screen shot 2017-08-22 at 19 03 15
BUT even if I comment them out from storybook config storybook looks broken - I think it is related to our theming setup - looks like css-variables are not loaded at all.
If you want we can check it out together tomorrow?

@DominikGuzei
Copy link
Member Author

@nikolaglumac i have fixed the remaining issues with storybook!

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Looks great now!
Approved ;)

@nikolaglumac nikolaglumac merged commit cbcec50 into master Aug 23, 2017
@nikolaglumac nikolaglumac deleted the feature/daef-300-exporting-wallet-to-file branch August 23, 2017 06:26
@nikolaglumac nikolaglumac restored the feature/daef-300-exporting-wallet-to-file branch August 23, 2017 06:37
@nikolaglumac nikolaglumac deleted the feature/daef-300-exporting-wallet-to-file branch August 23, 2017 06:46
@nikolaglumac nikolaglumac removed the WIP label Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants