Skip to content

Conversation

DeeJayElly
Copy link
Contributor

@DeeJayElly DeeJayElly commented Mar 17, 2021

This PR enables pasting of wallet recovery phase.

Todos

  • Improve logic to parse pasted strings (words)
  • Handle wrong words from the passphrase and break parsing of strings

Screenshots

Screen Shot 2021-03-18 at 7 48 55 AM


Testing Checklist


Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

@DeeJayElly DeeJayElly requested review from a team and DominikGuzei March 17, 2021 11:57
@DeeJayElly DeeJayElly self-assigned this Mar 17, 2021
Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

Hey @aleksandardjordjeviciohk, great work – the pasting works!
I added a few ideas how to make the logic more readable – also a few code comments could help here :)

One UX thing I noted: wasn't the idea that when you paste a series of words and one in the middle is incorrect, that this word is the last visible + the options popup shows "no results"? – at the moment only the correct words up until the first incorrect are pasted without any warning etc.

@DominikGuzei
Copy link
Member

DominikGuzei commented Mar 18, 2021

Hey @aleksandardjordjeviciohk, thanks for the update - the code looks cleaner now (still would use a few explanatory comments in the code though) … but I found a few ux bugs:

CleanShot 2021-03-18 at 20 52 46

CleanShot 2021-03-18 at 20 52 22

@DeeJayElly
Copy link
Contributor Author

Hey @aleksandardjordjeviciohk, thanks for the update - the code looks cleaner now (still would use a few explanatory comments in the code though) … but I found a few ux bugs:

CleanShot 2021-03-18 at 20 52 46

CleanShot 2021-03-18 at 20 52 22

There was an issue with not doing spread of previously selected items on the selections array. I have moved that line at the proper position now and that fixed these issues.

@nikolaglumac
Copy link
Contributor

@gabriela-ponce @daniloprates I would like to ask you to test this one too 🙏

DominikGuzei
DominikGuzei previously approved these changes Mar 19, 2021
@DominikGuzei
Copy link
Member

Looks good now and works as expected 🎉

@gabriela-ponce
Copy link

@aleksandardjordjeviciohk Overall, it looks good. These are some edge cases that I found:

  • Sometimes, when pasting the phrase with an invalid word, only the incorrect word is pasted. I couldn't find a clear pattern, but it usually occurs after switching the focus of the input. Check this video (I pasted the same phrase a few times, and in the last instance, only the incorrect word is displayed).

  • When there are some characters on the input, the phrase is pasted only partially and sometimes nothing is displayed. Check this video.

@DeeJayElly
Copy link
Contributor Author

Hey @aleksandardjordjeviciohk I found another possible UX issue: if there are multiple spaces between two words (instead of a single space) and you paste all of them only the words up to the multiple spaces are pasted (although all words have been correct). I guess that the logic for splitting the array could be improved to remove empty entries.

I have added the changes which cover this case, pls recheck @DominikGuzei

@nikolaglumac
Copy link
Contributor

@gabriela-ponce @DominikGuzei please review again 🙏

@miorsufianiohk
Copy link

LGTM @aleksandardjordjeviciohk 👍

gabriela-ponce
gabriela-ponce previously approved these changes Mar 25, 2021
Copy link

@gabriela-ponce gabriela-ponce left a comment

Choose a reason for hiding this comment

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

Looks good, nice work 💯

miorsufianiohk
miorsufianiohk previously approved these changes Mar 25, 2021
@nikolaglumac
Copy link
Contributor

@DominikGuzei we need your approval too 🙏

@nikolaglumac nikolaglumac self-requested a review March 26, 2021 16:07
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.

Dropdown does not close when you click on a word inside of it. The old version did...

@nikolaglumac
Copy link
Contributor

@miorsufianiohk @gabriela-ponce please re-test once @aleksandardjordjeviciohk fixes the issue I have reported here #163 (review)
Make sure to check those too. Thanks!

@DeeJayElly
Copy link
Contributor Author

Dropdown does not close when you click on a word inside of it. The old version did...

fixed

@DeeJayElly DeeJayElly requested a review from nikolaglumac March 29, 2021 09:40
@DeeJayElly DeeJayElly removed the WIP label Mar 29, 2021
@DeeJayElly
Copy link
Contributor Author

@DominikGuzei pls review this PR once again. If all good, we need to release new version and use it here input-output-hk/daedalus#2459

Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nikolaglumac
Copy link
Contributor

@DominikGuzei @aleksandardjordjeviciohk please merge this one and release next RP version so that we can integrate it in Daedalus 🙏

@DominikGuzei DominikGuzei merged commit 98decb0 into develop Mar 29, 2021
@DominikGuzei DominikGuzei deleted the feature/ddw-603-enable-pasting-of-wallet-recovery-phrase branch March 29, 2021 13:38
@DominikGuzei
Copy link
Member

released as 0.9.8-rc.13

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.

7 participants