-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix: unrecognized prop warning in form autosuggest #3003
fix: unrecognized prop warning in form autosuggest #3003
Conversation
✅ Deploy Preview for paragon-openedx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3003 +/- ##
=======================================
Coverage 93.18% 93.18%
=======================================
Files 249 249
Lines 4342 4342
Branches 1036 1036
=======================================
Hits 4046 4046
Misses 292 292
Partials 4 4 ☔ View full report in Codecov by Sentry. |
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 pointing out this bug! I believe the intended behavior is to set children
on the cloned element since the childChildren
variable used to just have the name children
paragon/src/Form/FormAutosuggest.jsx
Lines 75 to 82 in d32e6d0
return React.cloneElement(child, { | |
...rest, | |
children, | |
'data-value': children, | |
onClick: (e) => handleItemClick(e, onClick), | |
id: menuItemId, | |
onFocus: () => handleMenuItemFocus(menuItemId), | |
}); |
I've left suggestions that resolved the warning for me when testing locally.
src/Form/FormAutosuggest.jsx
Outdated
@@ -87,13 +87,13 @@ const FormAutosuggest = forwardRef( | |||
|
|||
function getItems(strToFind = '') { | |||
let childrenOpt = React.Children.map(children, (child) => { | |||
const { children: childChildren, onClick, ...rest } = child.props; | |||
const { children: childchildren, onClick, ...rest } = child.props; |
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.
const { children: childchildren, onClick, ...rest } = child.props; | |
const { children: childChildren, onClick, ...rest } = child.props; |
src/Form/FormAutosuggest.jsx
Outdated
childchildren, | ||
'data-value': childchildren, |
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.
childchildren, | |
'data-value': childchildren, | |
children: childChildren, | |
'data-value': childChildren, |
b3aa9ca
to
da34673
Compare
Hi @brian-smith-tcril, thanks for the suggestions. I have updated the PR. |
🎉 This PR is included in version 22.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
We encountered an issue where the
childChildren
prop passed to theMenuItem
component was flagged as an unrecognized prop to DOM (locally), leading to a console warning. Simultaneously, this warning caused the occurrence of duplicate API calls within our MFE (frontend-app-authn).Resolution: We renamed the
childChildren
tochildchildren
and the warning stopped appearing and the duplicate API calls issue was resolved.We are still unsure why we were having duplicate API calls due to this warning. Maybe React was trying to re-render due to invalid props? But we made sure that there was no issue with our implementation and it was due to the unrecognized prop. But we would like to understand the actual reason behind this if possible.
Console Warning screenshot.
Before
In this video, we can notice that we are getting single API calls until we click on the
FormAutoSuggest
field. On clicking the field, we get the console warning and all our API calls start firing twice.Screen.Recording.2024-02-13.at.12.56.25.PM.mov
After
In this video, (after the fix) we can see that we are neither getting console warnings nor duplicate API calls. All the API calls are being fired once.
Screen.Recording.2024-02-13.at.12.54.33.PM.mov
Deploy Preview
https://deploy-preview-3003--paragon-openedx.netlify.app/
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist