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

Final register ui change - Closes #1835 #1898

Merged
merged 12 commits into from Apr 18, 2019

Conversation

Projects
None yet
4 participants
@michaeltomasik
Copy link
Member

commented Apr 5, 2019

What issue have I solved?

#1835

How have I implemented/fixed it?

How has this been tested?

Review checklist

@michaeltomasik michaeltomasik self-assigned this Apr 5, 2019

@michaeltomasik michaeltomasik requested a review from massao Apr 5, 2019

@slaweet slaweet changed the base branch from 1.15.0 to 1.16.0 Apr 5, 2019

michaeltomasik added some commits Apr 7, 2019

@massao
Copy link
Contributor

left a comment

just some minor stuffs.
Also check that all spacings are correct with the layout, like the choose avatar has to little spacing when an avatar is selected:
Screen Shot 2019-04-09 at 09 51 38

Show resolved Hide resolved src/components/app/mixins.css Outdated
Show resolved Hide resolved src/components/app/mixins.css
Show resolved Hide resolved src/components/registerV2/accountCreated.css Outdated
Show resolved Hide resolved src/components/registerV2/backupPassphrase.css Outdated
Show resolved Hide resolved src/components/registerV2/backupPassphrase.css Outdated
Show resolved Hide resolved src/components/registerV2/confirmPassphrase.css
Show resolved Hide resolved src/components/registerV2/confirmPassphrase.css
Show resolved Hide resolved src/components/registerV2/confirmPassphrase.css
Show resolved Hide resolved src/components/registerV2/registerV2.css Outdated
Show resolved Hide resolved src/components/registerV2/registerV2.css Outdated
@massao
Copy link
Contributor

left a comment

👍

@massao massao requested a review from Efefefef Apr 10, 2019

@Efefefef
Copy link
Contributor

left a comment

Please change the wordings according to the document

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@michaeltomasik This looks different. Reasons why?

image

image

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

There is e2e skipping in this PR. Please outline reasons

michaeltomasik added some commits Apr 15, 2019

@@ -17,7 +17,7 @@ describe('Registration', () => {
* Create an account
* @expect Account is created
*/
it('Create an account', function () {
xit('Create an account', function () {

This comment has been minimized.

Copy link
@michaeltomasik

michaeltomasik Apr 15, 2019

Author Member

The design changed and there is no more account passphrase rendered as a string
cy.get(ss.copyPassphrase).invoke('text').as('passphrase');

This comment has been minimized.

Copy link
@michaeltomasik

michaeltomasik Apr 15, 2019

Author Member

@massao Suggestion

  /**
   * Create an account
   * @expect Account is created
   */
  it('Create an account', function () {
    cy.visit(urls.register);
    cy.get(ss.chooseAvatar).first().click();
    cy.get(ss.getPassphraseButton).click();
    this.passphrase = [];
    cy.get(ss.copyPassphrase).each(($el) => {
      this.passphrase = [...this.passphrase, $el.val()];
    });
    cy.get(ss.itsSafeBtn).click();
    cy.get(ss.passphraseWordConfirm).each(($el) => {
      if (this.passphrase.includes($el[0].textContent)) cy.wrap($el).click();
    });
    cy.get(ss.passphraseConfirmButton).click();
    cy.get(ss.loginBtn).click();
  });

This comment has been minimized.

Copy link
@massao

massao Apr 15, 2019

Contributor

@Efefefef Since we don't have the entire passphrase in only one element anymore, what do you think of the solution above? Or maybe you have a better solution to this case.

@michaeltomasik Also would have to update the selectors.js file, so the ss.copyPassphrase it like this: copyPassphrase: '.passphrase input'

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 16, 2019

Contributor

@michaeltomasik I think that's the legitimate way

Update src/components/registerV2/backupPassphrase.js
Co-Authored-By: michaeltomasik <shugo12345@gmail.com>

@massao massao requested a review from Efefefef Apr 15, 2019

@Efefefef
Copy link
Contributor

left a comment

@michaeltomasik
🐛 Accodring to designs it should be Paper Wallet, not Paper version. In the same time in a tooltip we have Paperwallet as approved text @reyraa Please clarify the terminology
🐛 The time in pdf file name should be in 24 hour format, not in 12
And please change the e2e test as discussed

@slaweet

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

  • 🐛 Selected avatar has wrong styles (size, shadow, white border size)
    Screenshot 2019-04-16 at 15 11 37
  • 🐛"Save" step is way too stretched vertically (doesn't fit into window on size where design does)
    Screenshot 2019-04-16 at 15 10 44
  • 🐛 avatar and "Address" label has wrong styles
    Screenshot 2019-04-16 at 15 12 35

michaeltomasik added some commits Apr 17, 2019

@slaweet slaweet changed the base branch from 1.16.0 to development Apr 18, 2019

@slaweet
Copy link
Member

left a comment

Thank you, Mike.

Feedback implemented

@slaweet slaweet added the ready label Apr 18, 2019

@michaeltomasik michaeltomasik merged commit 68776a5 into development Apr 18, 2019

5 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage remained the same at 94.148%
Details
security/snyk - package.json (LiskHQ) No manifest changes detected

@michaeltomasik michaeltomasik deleted the 1835-final-register-ui-change branch Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.