-
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
Make Angular component consumable multiple times #1657
Conversation
f6a0498
to
80ee94e
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.
Overall the changes look good, but there seem to be some problems now with the example app:
- there's no validation. Open any example with a required property (e.g. Day 2) and remove the data from the required field -> no errors are shown
- some examples don't work anymore, e.g. Day 1 now shows
No applicable renderer found!
d8232b4
to
5c9ecbc
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.
Thanks, looks good now!
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.
Looks good in general! Can you add a small description in the migration guide?
I assume an update to the Angular seed app is then required? Could you provide the changes in a PR so we can test them locally (for example via yalc)? |
5c9ecbc
to
90851c4
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.
I had a look! Looks good but I also saw some things which I missed on the first review.
8ad6906
to
5a0e291
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.
Found two problems in the new code.
5a0e291
to
fbcb8d9
Compare
fbcb8d9
to
abc8b6f
Compare
The JSONForms Angular component needs the jsonforms service to set the right data, schema, uischema etc. The problem is, that the service is a singleton, as it is registering as a root service. This commit introduces a new root component `jsonforms` which allows to pass all parameters into the component. The component is now initializing the service internally. Furthermore, the service is not registered as a root service anymore. Now multiple jsonforms components can be used next to each other. The angular tests were adapted to work with the behavior change.
abc8b6f
to
2335e64
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
The JSONForms Angular component needs the jsonforms service
to set the right data, schema, uischema etc.
The problem is, that the service is a singleton,
as it is registering as a root service.
This commit introduces a new root component
jsonforms
which allows to pass all parameters into the component.
The component is now initializing the service internally.
Furthermore, the service is not registered as a root service anymore.
Now multiple jsonforms components can be used next to each other.