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

abbreviated enode, CopyToClipboard component #3131

Merged
merged 5 commits into from
Nov 3, 2016
Merged

Conversation

derhuerst
Copy link
Contributor

done because of https://github.com/ethcore/parity/pull/3108#issuecomment-258114170

Fixes #3124 partially. Noticed that it doesn't make sense to do all the refactoring in this PR, so will open another one that replaces homegrown code by CopyToClipboard everywhere.

enode

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M6-ui labels Nov 3, 2016
import Theme from '../Theme';
const { textColor, disabledTextColor } = Theme.flatButton;

export default class CopyToClipboard extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

General comment - would prefer the filename as "copyToClipboard.js" since that is actually the standard through the codebase. (Some differently named files have slipped-in along the way, obviously none of us was paying attention).

);
}

onCopy = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really nice start. Only thing missing (maybe log an issue) is that in some places we have the SnackBar popping up, the component should do the same on copy. Apart from that, it will clean things up nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I forgot that.

@@ -68,9 +69,14 @@ class Status extends Component {
return null;
}

const [protocol, rest] = enode.split('://');
const [id, host] = rest.split('@');
const abbreviated = `${protocol}://${id.slice(0, 3)}…${id.slice(-3)}@${host}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Definately better. Looking at it now, we should probably just move it completely to the status page and not have it here at all - as you suggested earlier, it is not really user-facing.

@jacogr
Copy link
Contributor

jacogr commented Nov 3, 2016

Minor comments.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 85.912% when pulling 66f8a74 on abbreviated-enode into 2ed237a on master.

@gavofyork gavofyork 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
@gavofyork
Copy link
Contributor

filename changed according to @jacogr 's suggestion. moving the enode to the status page can probably happen in a second PR. @derhuerst you can log an issue for the "SnackBar popping up".

@jacogr
Copy link
Contributor

jacogr commented Nov 3, 2016

Agreed with the 2 issues, didn't mean in this PR.

Move enode to status page - https://github.com/ethcore/parity/issues/3151
Component to add snackbar - https://github.com/ethcore/parity/issues/3152

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 85.931% when pulling 1bcc71b on abbreviated-enode into 2ed237a on master.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Nov 3, 2016
@gavofyork gavofyork merged commit c79e328 into master Nov 3, 2016
@gavofyork gavofyork deleted the abbreviated-enode branch November 3, 2016 21:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants