-
Notifications
You must be signed in to change notification settings - Fork 1
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(AnyOfControl): fix missing default combinator value bug #75
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
+ Coverage 76.52% 76.78% +0.25%
==========================================
Files 36 36
Lines 409 435 +26
Branches 69 75 +6
==========================================
+ Hits 313 334 +21
- Misses 76 77 +1
- Partials 20 24 +4 ☔ View full report in Codecov by Sentry. |
const [renderCount, setRenderCount] = useState(1) | ||
|
||
useEffect(() => { | ||
// this is a janky workaround for two problems |
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 the plan to work on the fix in @jsonforms/react next?
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.
No, I'm going to file a bug though. I'm not interested in fixing it upstream because ideally we have a different approach altogether for default creation.
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.
What we're doing in the closed-source renderer is worse:
- we're giving the FormItem a name for a field that doesn't correspond to a property we want in the form
- we're setting a value for that field in a useEffect so form validation doesn't complain (TBC I made this change back in December when the bug was blocking release of splitters)
handleChange(path, newData) | ||
} | ||
} | ||
if (renderCount === 1) { |
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.
does this count work properly?
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.
yep! this code makes the test pass
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 approving. I see what you did but I still have questions.
- How it wasn't an issue in the mercury-web?
- Did you happen to have a chance to prove this is working in Add Asset form?
🎉 This PR is included in version 1.15.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Yes, I built the package locally and installed it to ensure it behaves as expected. Also my PR in mercury-web introduces a test that fails without the change included in this version. |
This PR fixes a couple problems: