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

fix: refactor see 24 words screen #162

Merged
merged 10 commits into from
Feb 25, 2022
Merged

fix: refactor see 24 words screen #162

merged 10 commits into from
Feb 25, 2022

Conversation

lucachaco
Copy link
Contributor

@lucachaco lucachaco commented Feb 22, 2022

New screenshots:
image

The white button is temporal to enable navigation. It will be removed when the new footer is implemented

Old screenshot:
image

Copy link
Member

@jessgusclark jessgusclark left a comment

Choose a reason for hiding this comment

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

The code looks good and thanks for leaving a note about the button being removed in the future. For the buttons and the styles a few comments:

  1. column should be left aligned so that the numbers line up
  2. the words need a borderRadius and need more padding (10 perhaps?)
  3. more padding/margin at the top of the app before the heading
    • more padding/margin below the heading
  4. make the overall font size a touch bigger for the numbers and words

Screen Shot 2022-02-23 at 1 37 45 PM

@lucachaco
Copy link
Contributor Author

lucachaco commented Feb 23, 2022

  1. Alligned to left.
  2. It had a border radius. For some reason was not working for IOS. After researching a found as solutions.
  3. Changed, but, unfortunately i cant give much more margin. Please remember you are using a design of 12 words instead of 24.
  4. Changed, but I cant make it much bigger in order to fit in a small device like in the screenshot in android

@ilanolkies ilanolkies added onboarding Onboarding ui User interface labels Feb 24, 2022
@ilanolkies ilanolkies added this to the v1.0.0 milestone Feb 24, 2022
Copy link
Contributor

@ilanolkies ilanolkies left a comment

Choose a reason for hiding this comment

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

PR looks really good to me. I didn't run it yet, just few comments on the code.

On older commetns:

  1. Agree
  2. Great! Now that we are working strictly on UI, I would go through the PRs in both operating systems. If no solution is found, please mark it in the code and will see in further iterations. We need to focus on delivering both devices ok but it is not an easy work so we might need to re-iterate
  3. and 4. "cant give much more margin" seems correct -- also, "make the overall font size a touch bigger" and "in order to fit in a small device": agree with both too. Let's keep this sizes and request better UI design increasing on accessibility

src/ux/createKeys/new/NewMasterKeyScreen.tsx Show resolved Hide resolved
src/ux/createKeys/new/NewMasterKeyScreen.tsx Show resolved Hide resolved
src/ux/createKeys/new/NewMasterKeyScreen.tsx Show resolved Hide resolved
src/ux/createKeys/new/NewMasterKeyScreen.tsx Outdated Show resolved Hide resolved
Comment on lines 37 to 38
{rows.map((row, i) => (
<View style={grid.row}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{rows.map((row, i) => (
<View style={grid.row}>
{rows.map((row, i) => {
const columnWordIndex = [row, row + rows.length, row + rows.length * 2]
return (
<View style={grid.row}>

This will prevent rewriting word index calculation, then use columnWordIndex[0], columnWordIndex[1] and columnWordIndex[2]

Copy link
Contributor

Choose a reason for hiding this comment

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

This, together with the other comment, will make code much cleaner and editable IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is the same since i then would need to use row -1 to access the words array. I rather start from 0

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment was more like thinking on preventing writing row + rows.length * column twice. So calculating what goes in each column would be better.

wordNumber={thing + 1}
text={words[thing]}

Still, we can see it later no worries

Copy link
Contributor

Choose a reason for hiding this comment

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

Now also thinking why we align things on rows and not in columns. We can use @jessgusclark 's grid system. See https://github.com/rsksmart/swallet/blob/ad4c79849437a25d951ad56489601603829abe9f/src/styles/grid.ts#L37-L40

Copy link
Contributor

Choose a reason for hiding this comment

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

Opening a refactoring issue. No need to see all this now. It is pretty clear how it works

src/ux/createKeys/new/NewMasterKeyScreen.tsx Outdated Show resolved Hide resolved
src/ux/createKeys/new/NewMasterKeyScreen.tsx Outdated Show resolved Hide resolved
lucachaco and others added 3 commits February 25, 2022 13:55
* 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>
Copy link
Contributor

@ilanolkies ilanolkies left a comment

Choose a reason for hiding this comment

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

Really nice job! Took time but it worth it

@ilanolkies ilanolkies dismissed jessgusclark’s stale review February 25, 2022 23:43

Addressed most of the things. We can loop back after new UI design is handed in

@ilanolkies ilanolkies merged commit bc4177c into develop Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
onboarding Onboarding ui User interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants