Skip to content

Conversation

DeeJayElly
Copy link
Contributor

@DeeJayElly DeeJayElly commented Apr 5, 2021

This PR enables pasting of wallet recovery phase.

This PR will utilized changes from React Polymorph Autocomplete component from here:
#163

JIRA Ticket: https://jira.iohk.io/browse/DDW-676

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

Pending issues:

1 - When deleting already entered words by clicking on the "x" button we should prevent the options dropdown to show (this is reproducible in develop too). Reported here (# 3).

2 - Choose "category" -> correctly enters the words in the input
Enter "cat" again -> it immediately picks "cat" without even submitting the words from the dropdown
So it wouldn't be possible to pick "category" again in this scenario. Reported here and related to this. More information here.

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

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.

@aleksandardjordjeviciohk I found a UX issue: when you paste words and one is incorrect it displays it correctly as the editable word + opens the dropdown. But when I choose the word from the dropdown, the incorrect/incomplete word stays editable / is still the last word - I would expect the incomplete word to be replaced by my choice in the dropdown instead

Uploading CleanShot 2021-04-06 at 19.56.47.mp4…

@gabriela-ponce
Copy link

@aleksandardjordjeviciohk I was able to replicate the issue @DominikGuzei mentioned in Storybook, but doesn't reproduce in Daedalus.

@DeeJayElly DeeJayElly requested a review from DominikGuzei April 14, 2021 08:52
…e' of github.com:input-output-hk/react-polymorph into feature/ddw-603-enable-pasting-of-wallet-recovery-phrase
@DeeJayElly
Copy link
Contributor Author

@aleksandardjordjeviciohk I found a UX issue: when you paste words and one is incorrect it displays it correctly as the editable word + opens the dropdown. But when I choose the word from the dropdown, the incorrect/incomplete word stays editable / is still the last word - I would expect the incomplete word to be replaced by my choice in the dropdown instead

Uploading CleanShot 2021-04-06 at 19.56.47.mp4…

Fixed @DominikGuzei

@DeeJayElly
Copy link
Contributor Author

@aleksandardjordjeviciohk I was able to replicate the issue @DominikGuzei mentioned in Storybook, but doesn't reproduce in Daedalus.

Can you retest once again now @gabriela-ponce ?

@gabriela-ponce
Copy link

@aleksandardjordjeviciohk The issue is fixed, but I found another odd behavior:

If you enter a word that starts like another word (for example "category" starts "cat") you can only insert the firsts letters that match the shorter word, meaning that you cannot insert the longer word manually, only by pasting it. Also if you paste part of the longer word, then when you start to delete the characters the input automatically selects the shorter word. Here's a video.

@DeeJayElly
Copy link
Contributor Author

@aleksandardjordjeviciohk The issue is fixed, but I found another odd behavior:

If you enter a word that starts like another word (for example "category" starts "cat") you can only insert the firsts letters that match the shorter word, meaning that you cannot insert the longer word manually, only by pasting it. Also if you paste part of the longer word, then when you start to delete the characters the input automatically selects the shorter word. Here's a video.

@gabriela-ponce i have updated the PR with a fix which covers this behavior. Pls recheck it.

@DeeJayElly DeeJayElly requested a review from a team April 14, 2021 21:28
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.

@aleksandardjordjeviciohk the issue with the words that start with the same letters is only half solved:
https://user-images.githubusercontent.com/172414/114856821-db77cd80-9de7-11eb-9e44-b08d425dfc2a.mp4

  1. Enter "cat" -> correctly shows "cat" and "category" in the dropdown
  2. Choose "category" -> correctly enters the words in the input
  3. Enter "cat" again -> it immediately picks "cat" without even submitting the words from the dropdown

So it wouldn't be possible to pick "category" again in this scenario

@gabriela-ponce
Copy link

@aleksandardjordjeviciohk I was able to replicate the issue that @DominikGuzei mentioned. Check this video.
Also, when you insert "cate" and delete the "e" the word "cat" gets selected automatically. Check this video.

@DeeJayElly
Copy link
Contributor Author

@aleksandardjordjeviciohk I was able to replicate the issue that @DominikGuzei mentioned. Check this video.
Also, when you insert "cate" and delete the "e" the word "cat" gets selected automatically. Check this video.

Fixed

…f-wallet-recovery-phrase' into feature/ddw-603-enable-pasting-of-wallet-recovery-phrase
@DeeJayElly DeeJayElly requested a review from DominikGuzei April 21, 2021 08:19
@gabriela-ponce
Copy link

Regarding this list of issues on #2459:

1 - When deleting an already entered words by using "Backspace" key we should always hide the options dropdown in case it is shown (this is reproducible in develop too):

1 - Fixed on Netlify

2 - When you enter the 2nd step of the "Wallet restoration" dialog, the button "Check recovery phrase" should be disabled by default (this is reproducible in develop too):

2 - Can't test on Netlify, doesn't reproduce on build 17687

3 - When deleting already entered words by clicking on the "x" button we should prevent the options dropdown to show (this is reproducible in develop too)

3 - Still reproduces on Netlify and build 17687. Check Video.

4 - When pasting recovery phrase on the "Verification of the recovery phrase" dialog, there is a JS error thrown (this does not happen in the "Wallet restoration" dialog):

4 - Can't test on netlify, doesn't reproduce on build 17687


Regarding these issues:

# 1
> Enter "cat" -> correctly shows "cat" and "category" in the dropdown

Choose "category" -> correctly enters the words in the input
Enter "cat" again -> it immediately picks "cat" without even submitting the words from the dropdown
So it wouldn't be possible to pick "category" again in this scenario

# 2

I was able to replicate the issue that @DominikGuzei mentioned. Check this video.
Also, when you insert "cate" and delete the "e" the word "cat" gets selected automatically. Check this video.

I can still reproduce them, and I found more details that can be relevant:

1 - Insert any word first
2 - Type "cat"

You'll notice that instead of displaying the list including "cat" and "category", the word "cat" is selected immediately. Check this video.

If you type "cat" and there are no other words on the input, the list will be displayed correctly. Check this video.

DominikGuzei
DominikGuzei previously approved these changes Apr 21, 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.

Looks good and works great now 👍

@DominikGuzei
Copy link
Member

@gabriela-ponce please re-check this, from what i saw the remaining issues have been fixed

gabriela-ponce
gabriela-ponce previously approved these changes Apr 22, 2021
@DeeJayElly DeeJayElly requested a review from DominikGuzei April 22, 2021 22:22
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.

I would like to discuss the changes involving timeouts and boolean flags - let's get rid of these by tackling the underlying issue together 🙏

this.setState((prevState) => ({ isRemoveWordClicked: !prevState.isRemoveWordClicked }));
}
}, 0);
}}
Copy link
Member

Choose a reason for hiding this comment

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

@aleksandardjordjeviciohk can we improve the isRemoveWordClicked logic to something cleaner? It looks like a hack to me and in general I would like to keep timeouts to the absolute minimum because they are also hacks in most cases.

@DominikGuzei
Copy link
Member

@aleksandardjordjeviciohk and I discussed that the Autocomplete component and especially this feature requires refactoring to avoid confusion and bugs in the future. The goals of this refactoring are to clean up the click/input handling to the latest UX we decided on and avoid bolting on more dumps with duck-tape.

@DominikGuzei DominikGuzei mentioned this pull request Apr 27, 2021
@nikolaglumac
Copy link
Contributor

@aleksandardjordjeviciohk and I discussed that the Autocomplete component and especially this feature requires refactoring to avoid confusion and bugs in the future. The goals of this refactoring are to clean up the click/input handling to the latest UX we decided on and avoid bolting on more dumps with duck-tape.

@DominikGuzei but we have merged Daedalus PR which depends on this PR input-output-hk/daedalus#2459

@nikolaglumac
Copy link
Contributor

@DominikGuzei @aleksandardjordjeviciohk why is this one still open? Is it still relevant?

@DominikGuzei
Copy link
Member

@nikolaglumac yes this is still relevant and I think @aleksandardjordjeviciohk is working on it. Unfortunately, we have merged a part of it already into Daedalus and I guess this PR was opened again because of some issues that have been found. When reviewing the changes I saw that the Autocomplete logic was in bad shape and didn't want to approve another quick fix on top of the existing bad code (not @aleksandardjordjeviciohk fault) … hopefully @aleksandardjordjeviciohk can tell us about the current state? In the worst case, I would revert the changes that already went into the 1.0.0 release from the previous PR

@nikolaglumac
Copy link
Contributor

@DominikGuzei please sort it out among the 3 of you. I would like to stay out of this discussion. Thanks!
cc @daniloprates @aleksandardjordjeviciohk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants