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

ui:Added dropdown for mnemonic phrase input #835

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

amitx13
Copy link
Contributor

@amitx13 amitx13 commented Sep 5, 2024

Fixes: #831

Added recommended word suggestions to the mnemonic phrase input field. The feature aims to improve the usability and accuracy of the mnemonic input process

To Test:
Navigate to Create new wallet or Import existing wallet.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

nice improvement

  • after entering word 12, the word suggestion is stuck

image

  • one nice addition might be to have enter keystroke select the suggestion when there is only one suggestion
  • at recovery selecting the suggestion will not automatically navigate the entry point to the next entry, while it does this at wallet creation?

@amitx13
Copy link
Contributor Author

amitx13 commented Sep 5, 2024

  • after entering word 12, the word suggestion is stuck

Hey @MarnixCroes, thanks for pointing that out—I appreciate your feedback. I will take a look into it.

  • one nice addition might be to have enter keystroke select the suggestion when there is only one suggestion

Sure, great suggestion! It would definitely be a nice addition.

  • at recovery selecting the suggestion will not automatically navigate the entry point to the next entry, while it does this at wallet creation?

Yes, I'm not sure, but I think it's because, at the time of wallet creation, JAM knows the mnemonic, but at the time of import, it doesn't. However, I will take a look and try to add this feature.

@amitx13 amitx13 requested a review from MarnixCroes September 5, 2024 21:48
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tACK 994f6ed
nice work

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Sep 7, 2024

tACK 994f6ed nice work

Second that!
tACK 994f6ed.
So nice. 🚀

Tests would be a nice to have.. 😉

@theborakompanioni
Copy link
Collaborator

  • one nice addition might be to have enter keystroke select the suggestion when there is only one suggestion

Sure, great suggestion! It would definitely be a nice addition.

One additional thing that can be done is a follow-up PR is, that the dropdown is not focused when "tab" is hit, when the full word has been written by the user. Currently, you have to press "tab" twice to go to the next input element, ideally, only one button press is need in such situations!
(Dropdown should still be displayed so the user can see that it is part of the wordlist! It should just not focus and immediately jump to the next input element).
What do you think?

editwentyone
editwentyone previously approved these changes Sep 9, 2024
Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

nice contribution, I tested it on safari macOS

@amitx13
Copy link
Contributor Author

amitx13 commented Sep 10, 2024

One additional thing that can be done is a follow-up PR is, that the dropdown is not focused when "tab" is hit, when the full word has been written by the user. Currently, you have to press "tab" twice to go to the next input element, ideally, only one button press is need in such situations! (Dropdown should still be displayed so the user can see that it is part of the wordlist! It should just not focus and immediately jump to the next input element). What do you think?

The dropdown remains visible but no longer gets focused when 'tab' is pressed after typing a full word. Now, pressing 'tab' moves directly to the next input element in these cases.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

eb040fc

at recovery when you write the full entry, the suggestion keeps being displayed. Even after the cursor entry is onto the next field.
It goes away when you start typing.
Not a big issue but a bit odd

Screencast.from.10-09-2024.13.31.45.webm

edit: this is the same as #835 (comment) afaict

@amitx13
Copy link
Contributor Author

amitx13 commented Sep 10, 2024

eb040fc

at recovery when you write the full entry, the suggestion keeps being displayed. Even after the cursor entry is onto the next field. It goes away when you start typing. Not a big issue but a bit odd

Yes, if you use Tab after writing a full word, it will show the dropdown so the user can see that it is part of the wordlist and navigate to the next input field.
Imho this is the right behaviour becz the purpose of Tab key is to navigate between focusable elements on a page, not for selecting option within a dropdown

edit: this is the same as #835 (comment) afaict

Am I understanding this correctly, @theborakompanioni ? Or is there something I’m missing?

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Sep 11, 2024

eb040fc
at recovery when you write the full entry, the suggestion keeps being displayed. Even after the cursor entry is onto the next field. It goes away when you start typing. Not a big issue but a bit odd

Yes, if you use Tab after writing a full word, it will show the dropdown so the user can see that it is part of the wordlist and navigate to the next input field. Imho this is the right behaviour becz the purpose of Tab key is to navigate between focusable elements on a page, not for selecting option within a dropdown

edit: this is the same as #835 (comment) afaict

Am I understanding this correctly, @theborakompanioni ? Or is there something I’m missing?

No, I think @MarnixCroes has a point here. It is good that tab jumps to the next input, but the dropdown should disappear. A user is already aware of that it is part of the worldlist if it is the only one displayed before tab is pressed. Makes sense?
Another thing: If there are multiple words to select, I need to press tab twice to select the first word. Can that be fixed?

Edit: Also, tests would be nice for this.
e.g. what happens if a user inputs "a" (multiple words), "aban" (only one word) or "abandon" (completely written out - only one word left) and then test the tab and tab + enter behaviour.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

27410d9
previous point is fixed #835 (review), this is smoother✅

Another thing: If there are multiple words to select, I need to press tab twice to select the first word. Can that be fixed?

agree that this could be an improvement as well. note that this is only when there is a scrollbar, when there are like just two suggestions then tab will go to the first suggestion at first press

@amitx13
Copy link
Contributor Author

amitx13 commented Sep 11, 2024

No, I think @MarnixCroes has a point here. It is good that tab jumps to the next input, but the dropdown should disappear. A user is already aware of that it is part of the worldlist if it is the only one displayed before tab is pressed. Makes sense?

yes

Another thing: If there are multiple words to select, I need to press tab twice to select the first word. Can that be fixed?

current behaviour: input (ja) -> dropdown appears -> pressed Tab once -> pressed Enter
I'm really sorry, I'm not getting this in one go. Could you please confirm, @theborakompanioni and @MarnixCroes, if this is not the behavior you both want?

Screencast.from.11-09-24.04.29.41.PM.IST.webm

Edit: Also, tests would be nice for this. e.g. what happens if a user inputs "a" (multiple words), "aban" (only one word) or "abandon" (completely written out - only one word left) and then test the tab and tab + enter behaviour.

Got it, but this can be done in a follow-up PR. As we discussed earlier, JAM is going to release a new version soon, so this would be a good addition. But if you need it now, I'm on it.

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Sep 11, 2024

No, I think @MarnixCroes has a point here. It is good that tab jumps to the next input, but the dropdown should disappear. A user is already aware of that it is part of the worldlist if it is the only one displayed before tab is pressed. Makes sense?

yes

Another thing: If there are multiple words to select, I need to press tab twice to select the first word. Can that be fixed?

current behaviour: input (ja) -> dropdown appears -> pressed Tab once -> pressed Enter I'm really sorry, I'm not getting this in one go. Could you please confirm, @theborakompanioni and @MarnixCroes, if this is not the behavior you both want?

Hmm.. it works in Chromium but works differently in Firefox. Here the dropdown itself is in focus after the first tab.

But it is not a deal-breaker and can be looked at in a follow-up PR.

Edit: Also, tests would be nice for this. e.g. what happens if a user inputs "a" (multiple words), "aban" (only one word) or "abandon" (completely written out - only one word left) and then test the tab and tab + enter behaviour.

Got it, but this can be done in a follow-up PR. As we discussed earlier, JAM is going to release a new version soon, so this would be a good addition. But if you need it now, I'm on it.

Yeeah, that is what most of the time rarely happens when it comes to tests in a follow-up PR 😉
Ideally, you'll write them before or as you develop the feature. Afterwards is quite hard as you have not written the code with tests in mind. But I am always open for surprises : -)

@MarnixCroes
Copy link
Contributor

Merge as is and open an issue for the follow ups?

@theborakompanioni
Copy link
Collaborator

Merge as is and open an issue for the follow ups?

Okay with that. 🙌

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Oct 16, 2024

@MarnixCroes @amitx13 Needs rebasing and some small fixes. I am preparing a patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI:Enhance Mnemonic Input Field with Recommended Words Suggestions
4 participants