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

Unsubscribe error on ShapeShift modal close #4005

Merged
merged 4 commits into from
Jan 4, 2017
Merged

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 30, 2016

  • allow for double unsubscribe
  • tests for subscribe/unsubscribe
  • reproduce already-unsubscribed failure

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 30, 2016
@ngotchac
Copy link
Contributor

Would prefer to keep the unsubscribe call in the onUnmount as well

@jacogr
Copy link
Contributor Author

jacogr commented Dec 30, 2016

Doesn't make sense having it in both places, however with the introduced flag it cannot hurt.

However, going to take a fresh look at this instead - first need a test that reproduces the error before fixing, so we can be sure instead of (probably) coming back at some point. A bit blind here and this may or may not do the trick, which is not acceptable.

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 30, 2016
@jacogr jacogr changed the title unsubscribe in onClose (state available) Unsubscribe error on ShapeShift modal close Dec 30, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 31, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 31, 2016

@ngotchac As expected, looked in the wrong place. With auto-unsubscribe (which didn't use the proper unsubscribe function, just set the values null) we caught an error when completed and trying again in unmounting the component. Fixed at source.

@ngotchac
Copy link
Contributor

ngotchac commented Jan 3, 2017

Looks good from a code point of view, however cannot really test fully with Shapeshift

@ngotchac
Copy link
Contributor

ngotchac commented Jan 4, 2017

Looks good. Would just get rid of the console.log here https://github.com/ethcore/parity/blob/jg-shapeshift-unsubscribe/js/src/modals/Shapeshift/shapeshift.js#L269 (just adds some noise IMO)

@ngotchac ngotchac 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 Jan 4, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Jan 4, 2017

Actually find that info quite useful, however that is for another PR since this one has not touched the modal, nor introduced that code - have one open where the modal is addressed, i.e. https://github.com/ethcore/parity/pull/4035

@ngotchac ngotchac 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 Jan 4, 2017
@jacogr jacogr merged commit 7cdfaf1 into master Jan 4, 2017
@jacogr jacogr deleted the jg-shapeshift-unsubscribe branch January 4, 2017 14:14
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.

2 participants