-
Notifications
You must be signed in to change notification settings - Fork 10
Fix: Don't display measureTable when measures is empty #341
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.
Thanks for digging into this!
Some minor feedback on the implementation.
For the tests, I am not entirely sure offhand. I must admit that the redux/jest workflow is kind of complex here. :/ Offhand your approach looks approximately correct, so I'm not sure what's gone wrong. What I normally do in this situation is insert a bunch of console.logs to try and understand what the system is doing and where data is flowing. Let me know if that helps -- if you can't figure it out let me know and I'll take a closer look.
frontend/ui/subview.jsx
Outdated
versions={this.props.versions} | ||
subviewState={this.state} | ||
/> | ||
{this.displayMeasuresTable()} |
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.
I don't think there's any need to delegate this to a method. I would keep this logic inline, and just do something like:
{
this.state.measures.crashMeasures.length && (
<MeasureTable
title="Crash Measures"
measures={this.state.measures.crashMeasures}
versions={this.props.versions}
subviewState={this.state}
/>)
}
(repeat this pattern for otherMeasures
)
@wlach The tests |
91b416b
to
07e7f88
Compare
@wlach issue fixed. Please review. |
@wlach could you please take a look at this? 😅 |
Sorry! Was away for a week and away from a computer and forgot to let you
know, will take a look first thing tomorrow.
…On Sun, Dec 9, 2018, 14:55 kagehina919 ***@***.*** wrote:
@wlach <https://github.com/wlach> could you please take a look at this? 😅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#341 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABQWQsqLm4xrS1_N34wW89UIHlWlRXoks5u3WqjgaJpZM4Y3cG9>
.
|
This looks and works great! I will merge. |
Added patch for #335.
I have added test for the same but the generated snapshot doesn't contain measureTable component. I've used mount instead of shallow rendering.
@wlach could you please help me with this?