Scopes to JSX#3611
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3611 +/- ##
==========================================
- Coverage 54.56% 54.51% -0.05%
==========================================
Files 120 120
Lines 4750 4747 -3
Branches 981 982 +1
==========================================
- Hits 2592 2588 -4
- Misses 2158 2159 +1
Continue to review full report at Codecov.
|
|
|
||
| function info(text) { | ||
| return dom.div({ className: "pane-info" }, text); | ||
| return ( |
There was a problem hiding this comment.
Hmm, to be more React idiomatic let's do.
function Info({ children }) { // or PaneInfo
return (
<div className="pane-info">
{children}
</div>
);
}| pauseInfo ? scopeInspector : info(L10N.getStr("scopes.notPaused")) | ||
| return ( | ||
| <div className="pane scopes-list"> | ||
| {pauseInfo ? scopeInspector : info(L10N.getStr("scopes.notPaused"))} |
There was a problem hiding this comment.
If the above is done this line should be
{pauseInfo ? scopeInspector : <PaneInfo>{L10N.getStr("scopes.notPaused")}</PaneInfo>}There was a problem hiding this comment.
Good call. Can actually do the same on ln 62 above.
There was a problem hiding this comment.
True you could make this block
<PaneInfo>
{pauseInfo ? L10N.getStr("scopes.notAvailable") : L10N.getStr("scopes.notPaused")}
</PaneInfo>
wldcordeiro
left a comment
There was a problem hiding this comment.
Some minor tweaks @ksaldana1 but thanks for all the great work!
c7c4d67 to
de80ea0
Compare
|
Thanks a lot @ksaldana1 you've been a big help on the JSX transforms. I've made a tweak to your last commit but this looks ready! I'll wait for CI and merge. |
jasonLaster
left a comment
There was a problem hiding this comment.
Looks good to me.
It would be nice to get some unit tests in there though :P
|
I agree, @jasonLaster. Are you wanting snapshot tests or more functional assertion unit tests? I guess the answer to that is probably both, ideally. I would like to become more familiar with Enzyme/Jest snapshots, so would be a good opportunity for me to swing back around on some of these more straightforward components. Happy to help @wldcordeiro! |
|
Yeah. Tests are really for working on components reliably. They're fun to
write too.
For scopes it's just snapshots. The interaction is in the Onject Indpector
…On Thu, Aug 10, 2017 at 12:01 AM Kevin Saldaña ***@***.***> wrote:
I agree, @jasonLaster <https://github.com/jasonlaster>. Are you wanting
snapshot tests or more functional assertion unit tests? I guess the answer
to that is probably both, ideally. I would like to become more familiar
with Enzyme/Jest snapshots, so would be a good opportunity for me to swing
back around on some of these more straightforward components.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3611 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPiYqw4nc3sOJxfb5G0WqmD6SdYK5x1ks5sWoCegaJpZM4OyuN_>
.
|
Associated Issue: #3506
Summary of Changes
Test Plan
Desk testing. No snapshot test to confirm against. Are we wanting to increase our snapshot coverage going forward?
Screenshots/Videos (OPTIONAL)