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: add ability to export network specifications from network setting screen #2360

Merged
merged 14 commits into from
Mar 26, 2020

Conversation

hanwencheng
Copy link
Contributor

@hanwencheng hanwencheng commented Mar 10, 2020

It would be nice if we could have an additional button to export the current network specifications to QR code, which could be used for Signer or otther places.

It rely on the PR on Polkadot.js/UI for display the networkSpecs QR code : polkadot-js/ui#299

How it works

After node connected, user could click the button, and the current network specs will be shown.

User could set the color with a 6-digit hex text input, or use a generate random button.

How to Test

Sidebar Select "Settings" Button -> Select "Metadata" Tab

Screen Shot 2020-03-26 at 18 20 41

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Will take a look when CI is happy, atm seems like a draft, so probably too early.

@hanwencheng
Copy link
Contributor Author

yeah, it is work in progress, I will update it in the weekend.

@hanwencheng hanwencheng force-pushed the hanwen-exportNetworkSpecs branch from c785ace to 59f8eb6 Compare March 23, 2020 13:40
@hanwencheng hanwencheng changed the title WIP: feat: add ability to export network specifications from network setting screen feat: add ability to export network specifications from network setting screen Mar 24, 2020
@hanwencheng hanwencheng requested a review from jacogr March 24, 2020 15:52
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Will go through, but I must admit that I am completely at a loss here. So we are generating a screen with random inputs that is not way tied to anything on chain and then get a QR to scan this?

@hanwencheng
Copy link
Contributor Author

The idea is if user is using their own node, and want to export the network information to Signer (related issue ) they will need to get the networrk info. It is convienient and trusted to directly use Polkadot.js to get these info.

Network info mainly can not be changed, whereas user is still capable to change name, path id and color.

@jacogr
Copy link
Member

jacogr commented Mar 24, 2020

Ok, the issues is that all that info is available in chain properties, ss58, decimals, etc. - yet we expect the user to input these? Does not make any sense to me.

@hanwencheng
Copy link
Contributor Author

Gensis Hash, Prefix, and Decimals are disabled Input Component, user cannot input them. I used Input component as convinieent and though it has same styling with the previous component like name, path Id and color.

@jacogr
Copy link
Member

jacogr commented Mar 24, 2020

Ok, so what is color meant to be? PathId?

(Colors are aligned with Polkascan based on chain only in apps, pathId has no meaning)

@hanwencheng
Copy link
Contributor Author

hanwencheng commented Mar 24, 2020

I did not realize that there is already ui-highlight-border class with chain color, now I use it as default color if it is a known network with 451fd12 . Color is the same as it is using here for UI to distinguish different chains. User can use custom color for it in case it is a custom node that not unknown to us.

Path ID is special for Signer, when user derive a network for an identity, the derived account will automatically prefixed with its pathId, e.g In Kusama network, path id is kusama, so every account derived will be prefixed with //kusama, like //kusama//funding, //kusama//staking, etc. The name is indeed hard to understand, or probably we should call it Path Prefix.

@jacogr
Copy link
Member

jacogr commented Mar 25, 2020

The "special to signer" stuff I would remove here. Atm it has not context in the Apps UI (paths are on the list somewhere here, as in the extension, but it really is app-specific. Passing stuff along is fine, but it needs to be known on both sides)

PS: Sorry fo the stoopid questions earlier, I missed the initial screenshot - and then (mistake #2), also missed the isDisabled in the code. So comedy of errors on my side.

@hanwencheng
Copy link
Contributor Author

Removed the pathId in commit 71effda , building test cannot pass because polkadot-js/ui#299 is not merged.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Some small comments.

packages/apps-config/src/ui/general/index.ts Outdated Show resolved Hide resolved
packages/page-settings/src/General.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/General.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
packages/page-settings/src/modals/NetworkSpecs.tsx Outdated Show resolved Hide resolved
@hanwencheng hanwencheng force-pushed the hanwen-exportNetworkSpecs branch from 71effda to 741d91a Compare March 26, 2020 17:25
@hanwencheng
Copy link
Contributor Author

hanwencheng commented Mar 26, 2020

Refactor Modal into "Metadata" tab, see the updated screenshot in PR description.

Thanks for adding useChainInfo hooker!

@hanwencheng hanwencheng requested a review from jacogr March 26, 2020 17:30
const _onSetRandomColor = (): void => setNetworkSpecs({ color: getRandomColor() });
const _checkColorValid = (): boolean => /^#[\da-fA-F]{6}|#[\da-fA-F]{3}$/.test(networkSpecs.color);

return <div
Copy link
Member

Choose a reason for hiding this comment

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

Can we just adapt this form to be -

return (
  <div className={className}>
...

@hanwencheng hanwencheng requested a review from jacogr March 26, 2020 20:26
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Thank you very much.

@jacogr jacogr merged commit 3f79f24 into polkadot-js:master Mar 26, 2020
@hanwencheng hanwencheng deleted the hanwen-exportNetworkSpecs branch March 26, 2020 21:20
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants