-
Notifications
You must be signed in to change notification settings - Fork 5
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/UX for repeting 24 words #163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comments in the other PR about alignment
Move WordInput
to its own component file. Let's try to keep our files as small as possible. This will help with readability for future developers
On my device it looks like this:
I was going to suggest that you have a 'disabled' state instead of showing a blank spot.
Also, you should be able to do the bottom purple background without needing the navigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Great job
I was going to suggest that you have a 'disabled' state instead of showing a blank spot.
I don't see it necessary. Again, we can ask for a better design later but nothing was defined about it. I would leave it as it is and spend our time in next work
Also, you should be able to do the bottom purple background without needing the navigation.
@jessgusclark what do you mean with this?
const selectWord = (selectedWord: string) => { | ||
const a = [...selectedWords, selectedWord] | ||
setSelectedWords(a) | ||
setWords(words.filter(word => !a.find(w => w === word))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this can make conflict with repeated words. If you click the second appearance of the word, the first will be removed 🤔 didn't test it out anyway
This takes me back some years when MyCrypto UI failed for me MyCryptoHQ/MyCrypto#2197
We need to remove them from the available words using the word index rather than the word as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow I didnt know we could have repeated words :O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The find() method returns only the first element in the provided array that satisfies the provided testing function. So, only one word will be removed. So we are good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I tested out the case. It works weird. I will open an issue
Screen.Recording.2022-02-25.at.20.31.03.mov
@lucachaco Regarding the purple (blue/color/etc) at the bottom. The design looks like this: The words to select/click should have a different background color than the top. |
Ok, now I get what you mean with purple (blue/color/etc) at the bottom comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!!
* fix: refactor see 24 words screen * fix: remove typo * fix: update navation name * fix: changes styles to make it on ios * fix: changes styles to make it on ios * fix: refactor word into component * fix: change styles * fix: change styles * fix: change colors * UI/UX for repeting 24 words (#163) * fix: confirm seed phrase * fix: shuffle words * fix: lint code * fix: change badge styles for IOS * fix: change styles * fix: change colors * Update src/ux/createKeys/new/ConfirmNewMasterKeyScreen.tsx Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com> Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com>
Merge first: #162
![image](https://user-images.githubusercontent.com/85146/155798774-120bc2fc-2495-4aa6-83d5-f4a1338909c4.png)
Button will be refactor with navigation ticket
Ok, I fixed the styles for IOS. Now working on the remaining small changes
New Screenshot:
Old screenshot:
![image](https://user-images.githubusercontent.com/85146/155247431-5de41bd9-9d0c-47ee-be13-3d1aa1f7cbe4.png)