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

feat: enable path derivation from existed path card #516

Merged
merged 4 commits into from
Jan 16, 2020

Conversation

hanwencheng
Copy link
Contributor

A small feature which ease the path creation process.

How to test:
Select Kusama network -> select any path -> tap top right menu button -> tap "Derive Account" button -> path derivation card should be a pre-filled path, and path creation should be correct.

At the same time, other path derivation should keep the same as before, which is:

  1. network path derivation: Network selection screen tap "Kusama" -> tap "Create New Derivation" button -> behavior same as before
  2. custom path derivation: Network selection screen tap "Add Network Account" -> Scroll to bottom and choose "Create Custom Path" -> behavior same as before

out

@hanwencheng hanwencheng added this to the v4.0 HDKD milestone Jan 7, 2020
@hanwencheng hanwencheng requested a review from Tbaut January 7, 2020 18:10
@hanwencheng hanwencheng self-assigned this Jan 7, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@hanwencheng hanwencheng force-pushed the hanwen-path-derivation-option branch from 7d58592 to 9ebfa68 Compare January 9, 2020 15:36
@hanwencheng
Copy link
Contributor Author

Also I am also thinking about the renaming of the button from Derive Account to Derive Child Account, what do you suggest @Tbaut ?

@Tbaut
Copy link
Contributor

Tbaut commented Jan 9, 2020

Regarding the naming (add "child"), I don't mind, not sure it clarifies anything though. One thing for sure, we need to unify the naming. We have "derive account", then on the button "derive address".

Regarding this PR, just looking at the presentation, I think it's confusing that the workflow is different (not a button but a menu entry) but in the end the behaviour is the same as the "derive account" button. I feel that for the same behaviour, we should have the same flow. Any thought on that?

@hanwencheng
Copy link
Contributor Author

Regarding the naming (add "child"), I don't mind, not sure it clarifies anything though. One thing for sure, we need to unify the naming. We have "derive account", then on the button "derive address".

True, will change that.

but in the end the behaviour is the same as the "derive account" button. I feel that for the same behaviour, we should have the same flow. Any thought on that?

The behavoir is different, take a look at the path, this derive path start with the path of the current select account, in the above example, it is //kusama//controller//nominating and user give /1 will derive account of //kusama//controller//nominating/1. It is like a syntax sugar for user with different and complex account.

Do not have any feature request from users, just personally think that would be useful.

@hanwencheng
Copy link
Contributor Author

Change the related text.

From the comments I realize that the derive base path is hard to distinguish. So in the commit:
7a5dbd3 I refactored the subtitle in the path derivation screen.

So now the subtitle always shows the base path of the derivation.

For example, in the flow:

  • select identity -> select kusama network -> select path "//kusama//default" -> "derive account" button -> show base path "//kusama//default"
  • select identity -> select kusama network -> tap "create new derivation" -> show base path "//kusama"
  • select identity -> tap "addn new network" -> tap "create custom path" -> show base path empty string ""

@Tbaut Tbaut merged commit 1368bca into master Jan 16, 2020
@Tbaut Tbaut deleted the hanwen-path-derivation-option branch January 16, 2020 13:34
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.

None yet

5 participants