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

Add LockID feature - Closes #496 #523

Merged
merged 8 commits into from Mar 16, 2018
Merged

Add LockID feature - Closes #496 #523

merged 8 commits into from Mar 16, 2018

Conversation

michaeltomasik
Copy link
Contributor

What was the problem?

Lack of LockID feature

How did I fix it?

Added LockID feature

How to test it?

Login to your account, go to switch accounts and click on Lock ID

Review checklist

  • The PR solves Add "Lock ID" feature #496
  • All new code is covered with unit tests
  • All new features are covered with e2e tests
  • All new code follows best practices

Copy link
Contributor

@yasharAyari yasharAyari left a comment

Choose a reason for hiding this comment

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

I think 'Lock ID' button should change to 'logout' because it actually covers logout functionality.

@slaweet
Copy link
Contributor

slaweet commented Mar 12, 2018

I think 'Lock ID' button should change to 'logout' because it actually covers logout functionality.

There will be a dedicated "Logout" button done in a separate issue: #540

@slaweet slaweet changed the base branch from development to 0.3.0 March 12, 2018 11:33
@@ -1,4 +1,10 @@
const account = {
lockDuration: 600000, // lock duration time is 10 minutes in milliSecond
genesis: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use /test/constants/accounts.js for this kind of purpose

e.stopPropagation();

const { savedAccounts } = this.props;
const savedActiveAccount = savedAccounts.find(acc => `${acc.network} ${acc.network}` === `${account.network} ${account.network}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. What if there are two accounts on the same network?

There should be some tests with multiple unlocked accounts that catch it.

step('Given I\'m on "account switcher" with accounts: "genesis,delegate,empty account"', setupStep);
step('Then I should see 3 instances of "saved account card"', () => helper.shouldSeeCountInstancesOf(1, 'strong.unlocked'));
step('When I click "Lock ID"', () => helper.clickOnElement('strong.unlocked'));
step('Then I should see 2 instances of "saved account card"', () => helper.shouldSeeCountInstancesOf(0, 'strong.unlocked'));
Copy link
Contributor

Choose a reason for hiding this comment

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

"Lock ID" should not remove the account, so there should still be 3 instances. I don't see how this test can pass.

},
};

module.exports = networks;
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be removed as a duplicate of src/constants/networks.js

<FontIcon value='unlocked' />
{t('Unlocked')}
{t('Lock ID')}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is one new requirement. When clicked, "Lock ID" should change to "Your ID is now secured!" and disappear only after ~3 seconds.

&:hover {
& span {
transform: scale(1.1);
transform-origin: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

The hover effect would be smoother with transition: transform ease-in-out 500ms;

@slaweet slaweet removed this from Pull Requests in Sprint Board 05-03-18 Mar 13, 2018
@slaweet slaweet added this to New Issues in Sprint Board 12-03-18 via automation Mar 13, 2018
@slaweet slaweet moved this from New Issues to Pull Requests in Sprint Board 12-03-18 Mar 13, 2018
@slaweet slaweet merged commit 9a7e00d into 0.3.0 Mar 16, 2018
Version 0.3.0 automation moved this from Pull Requests to Merged Pull Requests Mar 16, 2018
Sprint Board 12-03-18 automation moved this from Pull Requests to Merged Pull Requests Mar 16, 2018
@slaweet slaweet deleted the 496-add-lockid-feature branch March 16, 2018 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Sprint Board 12-03-18
  
Merged Pull Requests
Version 0.3.0
  
Merged Pull Requests
Development

Successfully merging this pull request may close these issues.

None yet

4 participants