-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Refactoring on exploreReducer.js #4836
Conversation
return Object.assign({}, state, { controls }); | ||
return { | ||
...state, | ||
...controls, |
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'm not sure if this line is right, shouldn't this be controls: just controls,
?
return Object.assign({}, state, { controls }); | ||
return { | ||
...state, | ||
...controls, |
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.
Is this right?
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.
Reference ^^ above comments
Please fix the build failure and merge conflict. |
Codecov Report
@@ Coverage Diff @@
## master #4836 +/- ##
==========================================
+ Coverage 76.91% 76.96% +0.04%
==========================================
Files 44 44
Lines 8522 8534 +12
==========================================
+ Hits 6555 6568 +13
+ Misses 1967 1966 -1
Continue to review full report at Codecov.
|
I was following the same pattern for the Object.assign which is merging state and controls together. I've refactored it using the spread operator notation.
|
return Object.assign({}, state, { controls }); | ||
return { | ||
...state, | ||
...controls, |
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.
From my understanding this will spread the content of controls
into the root of state
. Meaning if I translate your current statement here into the pre-ES6 notation I'll get Object.assign({}, state, controls);
which is different from Object.assign({}, state, { controls });
, we want the controls to go under state.controls
, not in the root of state
LGTM |
* refactoring on exploreReducser * fix building json
* refactoring on exploreReducser * fix building json
* refactoring on exploreReducser * fix building json
Changed return statement to have es6 syntax insteas of
Object.assign()
function call@betodealmeida @mistercrunch