-
Notifications
You must be signed in to change notification settings - Fork 6
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
[OF#5086] Fix showing soft requirement warning in hidden components #795
base: main
Are you sure you want to change the base?
Conversation
Bundle ReportChanges will increase total bundle size by 146 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: @open-formulieren/sdk-OpenForms-umdAssets Changed:
Files in
view changes for bundle: @open-formulieren/sdk-esmAssets Changed:
Files in
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #795 +/- ##
==========================================
+ Coverage 83.90% 84.18% +0.27%
==========================================
Files 246 246
Lines 4816 4819 +3
Branches 1282 1282
==========================================
+ Hits 4041 4057 +16
+ Misses 746 733 -13
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
965f4a3
to
40028f1
Compare
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.
Few minor remarks/questions :)
40028f1
to
3b142ff
Compare
@@ -33,7 +33,11 @@ class SoftRequiredErrors extends FormioContentField { | |||
// check which components have an empty value | |||
for (const component of softRequiredComponents) { | |||
const isEmpty = component.isEmpty(); | |||
if (isEmpty) missingFieldLabels.push(component.label); | |||
const isParentVisible = component.parent?.visible; |
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 it the grand parent is hidden?
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.
Yes, you are right I didn't think about it.
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.
After some research I saw that even if we have nested components this works. If, as you mentioned, the grand parent is hidden, all the children are hidden as well. I understood that formio (the UI) does not render the children of a hidden component, even if the json configuration has hidden=false
.
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.
That's correct - I just didn't know if component.visible
took into account is parents or not :) So, do we then need the isParentVisible
check or not? I'd expect that component.visible
already checks that, no?
…ning in hidden components
3b142ff
to
cea8518
Compare
Fixes open-formulieren/open-forms#5086