Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time Travel: unmount in the shutdown() action #2801

Merged
merged 2 commits into from
Aug 10, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Aug 8, 2017

Resolves https://github.com/weaveworks/service-ui/issues/814 by making timeline deconstruction part of the shutdown() action, avoiding the attempt to establish new connections.

Also, triggering resumeTime action only if previously paused.

@fbarl fbarl self-assigned this Aug 8, 2017
@rade rade requested a review from jpellizzari August 9, 2017 19:01
@jpellizzari
Copy link
Contributor

jpellizzari commented Aug 9, 2017

LGTM, without any context, but still has the [WIP] tag.

@fbarl fbarl force-pushed the dont-resume-time-when-timeline-unmounted branch from d58dab2 to 0c78962 Compare August 10, 2017 14:36
@fbarl fbarl changed the title [WIP] Resume time works only if previously paused. Time Travel: unmount in the shutdown() action Aug 10, 2017
@fbarl
Copy link
Contributor Author

fbarl commented Aug 10, 2017

@jpellizzari PTAL

LGTM, without any context, but still has the [WIP] tag.

I think the first commit accidentaly solved the case described in the referenced issue, but you'd still get the same bug if you tried clicking on Time Travel before switching to Monitor - now that part has been fixed as well.

p.s. the branch name is still a bit of a misnomer :)

Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Code LGTM

@fbarl fbarl merged commit badac90 into master Aug 10, 2017
@fbarl fbarl deleted the dont-resume-time-when-timeline-unmounted branch August 11, 2017 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants