Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Use enode RPC in UI #3108

Merged
merged 6 commits into from
Nov 3, 2016
Merged

Use enode RPC in UI #3108

merged 6 commits into from
Nov 3, 2016

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Nov 2, 2016

Closes #3106

Gets the enode address from the RPC call and display it.
Added it to JS API.

Needs #3096 to be merged.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M6-ui labels Nov 2, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 2, 2016

  1. Merge in master first - the PR has been merged, so now shows Rust changes.
  2. Missing addition of interface in jsonrpc

@jacogr jacogr added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 2, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 86.002% when pulling 72bbdb6 on ng-enode-rpc into 36d17d5 on master.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Nov 3, 2016
const nextTimeout = (timeout = 1000) => {
setTimeout(this._pollStatus, timeout);
};

if (isConnected !== this._store.getState().nodeStatus.isConnected) {
Copy link
Contributor

@derhuerst derhuerst Nov 3, 2016

Choose a reason for hiding this comment

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

const wasConnected = this._store.getState().nodeStatus.isConnected;
if (isConnected !== wasConnected) 

@derhuerst
Copy link
Contributor

derhuerst commented Nov 3, 2016

I think monospaced text will be better to read in this case.

Also, will the enode actually be relevant to the majority of the users? This is so much text that will potentially be just noise to many users. What about putting it in a tooltip on the "x/y/z peers" text?

@derhuerst derhuerst added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 3, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 3, 2016

Sadly we don't use monospaced fonts on the status page atm, so it would completely out of place if added. On the status page we have all the technical status settings, which is why it is not visible to users. (e.g. miner setting, mining author, ports, etc.) So the positioning is correct.

Happy with what is there overall, right place, right positioning.

@gavofyork
Copy link
Contributor

gavofyork commented Nov 3, 2016

i'd like to get rid of long hex strings from the UI in general. this includes at least addresses and enode ids.

they're easily replaced by a copy button (in case the user actually wants them) - possibly mouse-over sensitive, and a rendering with just the first few nibbles on the front and back e.g. enode://abcdef...abcd@127.0.0.1:30303

@gavofyork gavofyork merged commit d99f1b5 into master Nov 3, 2016
@gavofyork gavofyork deleted the ng-enode-rpc branch November 3, 2016 11:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants