Enhancement/11296 enhance use form value#12648
Conversation
|
Size Change: 0 B Total Size: 2.3 MB ℹ️ View Unchanged
|
abdelmalekkkkk
force-pushed
the
enhancement/11296-enhance-use-form-value
branch
from
0fd5e96 to
2e6b53b
Compare
|
@abdelmalekkkkk please address the merge conflict here, thanks. |
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @abdelmalekkkkk, this looks great. Besides the merge conflict there are just a few points I've noted for your review.
|
|
||
| const resetUnstagedSelection = useCallback( () => { | ||
| setValues( KEY_METRICS_SELECTION_FORM, { | ||
| [ KEY_METRICS_SELECTED ]: selectedMetrics, |
There was a problem hiding this comment.
[ KEY_METRICS_SELECTED ] is not set in the refactored version
There was a problem hiding this comment.
It was setting KEY_METRICS_SELECTED to its current value selectedMetrics, which looks unnecessary, unless I'm missing something?
There was a problem hiding this comment.
That makes sense, just highlighting as a potential oversight.
There was a problem hiding this comment.
After taking a closer look, let's double check this since this is also updated with another value currentlySelectedMetrics in other places so it may be intentional after all.
There was a problem hiding this comment.
I double checked and it still seems unnecessary. The one in assets/js/components/KeyMetrics/MetricsSelectionPanel/MetricItem.js (which I think is what you referred to) makes sense because it updates KEY_METRICS_SELECTED based on which checkbox was toggled, either appending to or removing from the existing selected metrics array.
| setValues( AUDIENCE_TILE_CUSTOM_DIMENSION_CREATE, { | ||
| autoSubmit: false, | ||
| isRetrying: false, | ||
| } ); |
There was a problem hiding this comment.
This happens in a few places, but anywhere we're splitting a single state change into two state changes, there is the possibility of something breaking in the intermediate state (e.g. autoSubmit is false, but isRetrying is still true. This is because each state change will trigger a render. React batches updates, so that is less of an issue – we just need to make sure it doesn't introduce a problem where none was before.
In most places we're only dealing with a single key in the form state, but if needed, we can also keep the previous form for these scenarios. Another somewhat more complicated option would be to wrap the multiple calls with registry.batch.
We don't need to reach for this now but just mentioning the options we have in case the refactor is problematic in any of these places.
There was a problem hiding this comment.
Yes, I considered that while doing the refactor but figured it would probably not be a problem.
I actually came across one instance where splitting the state update caused a test to fail, which was the setValues in assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/Panel.js. The actual UI seemed to work fine but a couple of tests failed. I just let that setValues as it is since debugging those tests seemed like it would take a lot more time.
abdelmalekkkkk
force-pushed
the
enhancement/11296-enhance-use-form-value
branch
from
07370ab to
b3c9535
Compare
|
Thanks @aaemnnosttv. I resolved the merge conflicts and replied to your comments. Please take another look. |
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @abdelmalekkkkk – one thing to double check otherwise this LGTM
See #12648 (comment)
abdelmalekkkkk
force-pushed
the
enhancement/11296-enhance-use-form-value
branch
from
b3c9535 to
3fbbe3a
Compare
|
Thanks @aaemnnosttv. I did a double check and it still looks fine to me. I fixed the position of one comment that I misplaced during the refactor. The merge conflict is also resolved. |
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Great, thanks @abdelmalekkkkk !
2 participants
Summary
Addresses issue:
useFormValueto get and set #11296Relevant technical choices
Slight deviation from IB: did not add a new action
setValuesince the existingsetValuescould be used just fine.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist