-
Notifications
You must be signed in to change notification settings - Fork 373
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: resolve refs in EnumArrayRenderer #2204
fix: resolve refs in EnumArrayRenderer #2204
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Hi @LukasBoll the changes look pretty good to me. There is one typo fix inline that also fixes the out of memory of the tests. I'm not sure why the typo led to an out of memory but it should not be related to this PR:
Likely it is ue to the implementation of mapStateToMultiEnumControlProps
. However, I did not see anything obvious there. Furthermore, in a running application the method should never be invoked because the tester does not apply for effectively empty items
. I tested this in the example material app and there simply a No applicable renderer found.
is shown.
colors: { | ||
type: 'array', | ||
items: { | ||
ref$: '#/definitions/colors', |
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.
Fixing this typo makes the tests work again locally for me. I.e. it fixes the out of memory issue of the tests.
ref$: '#/definitions/colors', | |
$ref: '#/definitions/colors', |
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.
Oh, what a stupid mistake. The test is working now, thank you for taking a look @lucas-koehler !
I think the out of memory issues occurred in a view other test cases as well.
So maybe there is an issue with one of the core functionalities, that is called by mapStateToMultiEnumControlProps
.
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, I am also aware of at least on other PR with out of memory issues. Thanks for the hint. However, we don't need to look at that in this PR :D
resolve refs ind the MultiEnumControlProps mapper and tester fixes eclipsesource#2192
42b22a4
to
1265382
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.
LGTM now! Thanks for the contribution :)
resolve refs ind the MultiEnumControlProps mapper and tester
fixes #2192