-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Unsubscribe error on ShapeShift modal close #4005
Conversation
jacogr
commented
Dec 30, 2016
•
edited
Loading
edited
- allow for double unsubscribe
- tests for subscribe/unsubscribe
- reproduce already-unsubscribed failure
Would prefer to keep the |
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. |
@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. |
Looks good from a code point of view, however cannot really test fully with Shapeshift |
Looks good. Would just get rid of the |
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 |