[WhyPaused] Move why paused to the call stack#3119
Conversation
jasonLaster
left a comment
There was a problem hiding this comment.
Looking great. one small comment
| return dom.div( | ||
| { className: "pane frames" }, | ||
| this.renderFrames(frames), | ||
| WhyPaused(), |
There was a problem hiding this comment.
now that WhyPaused is a child of Frames, it would be nice to pass pauseInfo down WhyPause({ pauseInfo }). This means we no longer need to have the connect logic in WhyPaused too :)
There was a problem hiding this comment.
diff --git a/src/components/SecondaryPanes/WhyPaused.js b/src/components/SecondaryPanes/WhyPaused.js
index 5741743..d96978d 100644
--- a/src/components/SecondaryPanes/WhyPaused.js
+++ b/src/components/SecondaryPanes/WhyPaused.js
@@ -23,7 +23,7 @@ function renderExceptionSummary(exception) {
return `${name}: ${message}`;
}
-class WhyPaused extends Component {
+export default class WhyPaused extends Component {
renderMessage(pauseInfo: Pause) {
if (!pauseInfo) {
return null;
@@ -66,10 +66,3 @@ WhyPaused.displayName = "WhyPaused";
WhyPaused.propTypes = {
pauseInfo: PropTypes.object
};
-
-export default connect(
- state => ({
- pauseInfo: getPause(state)
- }),
- dispatch => bindActionCreators(actions, dispatch)
-)(WhyPaused);There was a problem hiding this comment.
Absolutely agree. Had a thought about making it a stateless component, but didn't know what do you guys think about that.
There was a problem hiding this comment.
Removing an unnecessary connect is a great idea. I think you can go ahead with making it stateless.
|
Also, looks like you need to rebase to fix our test on master and run |
|
Really nice! How does this look with the error message? I think we had trouble with a centered layout when there was also an error message. You can see those when you pause on exceptions. |
972d486 to
0b30920
Compare
Codecov Report
@@ Coverage Diff @@
## master #3119 +/- ##
===========================================
+ Coverage 47.63% 65.98% +18.35%
===========================================
Files 95 76 -19
Lines 4012 2699 -1313
Branches 821 544 -277
===========================================
- Hits 1911 1781 -130
+ Misses 2101 918 -1183
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3119 +/- ##
==========================================
+ Coverage 47.51% 47.54% +0.03%
==========================================
Files 95 95
Lines 4003 3998 -5
Branches 821 821
==========================================
- Hits 1902 1901 -1
+ Misses 2101 2097 -4
Continue to review full report at Codecov.
|
… into move-why-paused-to-the-call-stack
|
some tests are failing still and I don't get why. It looked like |
| * We now disable out of scope lines when the debugger pauses. | ||
| * We have huge updates to preview - it's faster, more consistent, and works for HTML elements | ||
| * Breakpoints are kept in sync as code changes. Big thanks to [@codehag] | ||
| * Breakpoints are kept in sync as code changes. Big thanks to [codehag][@codehag] |
There was a problem hiding this comment.
This was changed because lint-md was yelling on this. Maybe I was wrong here... Sorry
There was a problem hiding this comment.
no worries :D i think we all fixed it because it was wrong on master
|
It might be that not all the tests ran, and one of the tests that was out of date wasn't updated. I updated that right now... should be ok. lets see! |
|
Thanks @zaggy! This is a really nice style win. One small style nit, which we can do in a follow up is style the errors red. Here's a before and after with some of the CSS I used to sketch it before
after
CSS
|
Associated Issue: #3111
Summary of Changes
I definitely don't like the color of text in dark theme (it looks too visible), but I need your feedback on this.
Test Plan
Screenshots/Videos (OPTIONAL)