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

Cancel tx JS #4958

Merged
merged 45 commits into from
Apr 25, 2017
Merged

Cancel tx JS #4958

merged 45 commits into from
Apr 25, 2017

Conversation

CraigglesO
Copy link
Contributor

Javascript updates from #4949

  • updated jsonrpc->parity
  • updated api->rpc->parity
  • spelling/grammar fixes

img1
img2
img3

@CraigglesO CraigglesO added A0-pleasereview 🤓 Pull request needs code review. M7-signer 🔏 Trusted signer. labels Mar 18, 2017
npm-debug.log Outdated
@@ -0,0 +1,24 @@
0 info it worked if it ends with ok
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove.

@jacogr
Copy link
Contributor

jacogr commented Mar 19, 2017

Small sanitasion grumbles - Remove npm-debug.log (should probably be added to .gitignore), update with latest master (to pull in the changes from @tomusdrw , currently they still show in this PR)

Will play a bit, excited to have this in.

@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 19, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 19, 2017
@CraigglesO
Copy link
Contributor Author

FIXED! Good eye! I added to .gitignore for the future.

I'll definitely refactor the code when the edit button comes in.


if (canceled) {
return (
<div className={ styles.pending }>CANCELED</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

FormattedMessage?

if (!isCancelOpen) {
return (
<div className={ styles.pending }>
<div>SCHEDULED</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

FormattedMessage?

<div>SCHEDULED</div>
<a
className={ styles.cancel }
onClick={ () => this.setState({ isCancelOpen: true }) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer not using in-line closures, aligning with the codebase. (I actually thought we had a linting rule that warns about these, weird that it wasn't picked up)

<a onClick={ () => this.cancelTransaction(parity, hash) }>Cancel</a>
<span> | </span>
<a onClick={ () => this.setState({ isCancelOpen: false }) }>Nevermind</a>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

FormattedMessage & inline closures.

line-height: 1.5em;
opacity: 0.5;
color: grey;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure we use this anywhere, would prefer to keep it where it was to keep consistent (We really need to externalise all colours and just pull in CSS variables - no duplication, consistency is maintained)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is, if opacity is dropped, the "cancel" button will also be 50% opacity LOL. Also, all font is either black or white, 0.5 opacity for both black and white revert to grey anyways.

],
returns: {
type: Boolean,
desc: 'returns `true` if the upgrade to the new release was successfully executed, `false` if not.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Description needs updating

@@ -100,7 +100,7 @@ class RequestsPage extends Component {
<div className={ styles.noRequestsMsg }>
<FormattedMessage
id='signer.requestsPage.noPending'
defaultMessage='There are no requests requiring your confirmation.'
defaultMessage='There are no transactions or requests requiring your confirmation.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I 100% agree with this, although getting heavily into bikeshed territory :)
Transaction signing requests are still signer requests. "transactions or requests" implies that they aren't.

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Tiny typo in removeHash function causing the blocks to leak, but otherwise looks good.

@CraigglesO
Copy link
Contributor Author

This should do it. 👍

block = blockNumber.minus(block);
return (block.toNumber() < 0)
? block.abs().toFormat(0) + ' blocks left'
: 'submitting';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make the strings here translatable. (Also line 247 is still, as part of the value for which, non-translatable)

@jacogr
Copy link
Contributor

jacogr commented Apr 19, 2017

Couple of last i18n issues, but apart from that looking good.

@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 19, 2017
@CraigglesO
Copy link
Contributor Author

Let me know 💃

@CraigglesO CraigglesO added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 21, 2017
@jacogr jacogr added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 22, 2017
@jacogr
Copy link
Contributor

jacogr commented Apr 22, 2017

One last conflict.

@CraigglesO
Copy link
Contributor Author

DONE.

@jacogr jacogr merged commit f7d5d6c into master Apr 25, 2017
@jacogr jacogr deleted the cancel-tx-js branch April 25, 2017 08:08
@5chdn 5chdn mentioned this pull request May 10, 2017
@5chdn 5chdn added the B0-patch label May 18, 2017
@5chdn
Copy link
Contributor

5chdn commented May 18, 2017

Can we have this backported to beta? It's causing a support nightmare :)

tomusdrw pushed a commit that referenced this pull request May 18, 2017
* Remove transaction RPC

* Bumping multihash and libc

* Updating nanomsg

* bump nanomsg

* cancel tx

* cancel-tx-js

* cancel-tx-js

* cancel-tx-js

* cancel-tx-hs

* cancel-tx-js

* cancel-tx-js

* cancel-tx-js

* small fixes

* edit & time till submit

* edit & time till submit

* updates

* updates

* udpates

* udpates

* grumbles

* step 1

* Wonderful updates

* ready

* small refact

* small refact

* grumbles 1

* ffx2

* good ol' fashioned updates

* latest and greatest

* removeHash

* removeHash

* spec

* fix 1

* fix 1

* fix 2

* fix 2

* ff

* ff

* ff

* updates
arkpar pushed a commit that referenced this pull request May 18, 2017
* option to disable persistent txqueue (#5544)

* option to disable persistent txqueue

* New option goes with kin

* Remove transaction RPC (#4949)

* Cancel tx JS (#4958)

* Remove transaction RPC

* Bumping multihash and libc

* Updating nanomsg

* bump nanomsg

* cancel tx

* cancel-tx-js

* cancel-tx-js

* cancel-tx-js

* cancel-tx-hs

* cancel-tx-js

* cancel-tx-js

* cancel-tx-js

* small fixes

* edit & time till submit

* edit & time till submit

* updates

* updates

* udpates

* udpates

* grumbles

* step 1

* Wonderful updates

* ready

* small refact

* small refact

* grumbles 1

* ffx2

* good ol' fashioned updates

* latest and greatest

* removeHash

* removeHash

* spec

* fix 1

* fix 1

* fix 2

* fix 2

* ff

* ff

* ff

* updates

* Updating documentation for RPCs (#5392)

* Removing minBlocks occurrencies

* Docs for new RPCs.

* Fixing linting issues, updating *withToken documentatiojn.

* Adding missing RPCs. Fixing tests.

* Fixing lint issues.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. M7-signer 🔏 Trusted signer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants