[WhyPaused] Move why paused to the call stack #3119
[WhyPaused] Move why paused to the call stack #3119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. one small comment
@@ -176,6 +182,7 @@ class Frames extends Component { | |||
return dom.div( | |||
{ className: "pane frames" }, | |||
this.renderFrames(frames), | |||
WhyPaused(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@@ -107,7 +107,7 @@ This was a really great week for QA improvements as the debugger is getting more | |||
|
|||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed because lint-md was yelling on this. Maybe I was wrong here... Sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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 beforeafterCSS |
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)