-
Notifications
You must be signed in to change notification settings - Fork 419
Transform CategorizationRenderer from class to functional #2120
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
Transform CategorizationRenderer from class to functional #2120
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
sdirix
left a comment
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.
Please add a test case.
I don't think the error is fixed. The current category is still saved in state and is not updated on ui schema changes.
e27f976 to
b4397ac
Compare
f7eeb7c to
a24969c
Compare
|
Can you check the failing test cases? |
a24969c to
c34f52a
Compare
cbf727d to
c3d0927
Compare
eacc2ac to
ae99552
Compare
packages/vanilla-renderers/src/complex/categorization/CategorizationRenderer.tsx
Outdated
Show resolved
Hide resolved
lucas-koehler
left a comment
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 @TheZoker , thanks for the contribution.
I left some comments inline. Please have a look :) The two main issues atm seem to be:
- In
CategorizationList, and endless loop is created by handing down the samefilteredCategories. This causes the out of memory error during tests. More details are in the comment inline - The new
filteredCategoriesprop is of the wrong type and does not considerCategorizationelements despite them effectively being handed down. Handing them down seems to be correct because otherwise we could no longer handle nestedCategorizations. The type and potentially the prop name should be fixed accordingly.
packages/vanilla-renderers/src/complex/categorization/CategorizationList.tsx
Outdated
Show resolved
Hide resolved
packages/vanilla-renderers/src/complex/categorization/CategorizationList.tsx
Outdated
Show resolved
Hide resolved
packages/vanilla-renderers/src/complex/categorization/CategorizationRenderer.tsx
Outdated
Show resolved
Hide resolved
packages/vanilla-renderers/src/complex/categorization/CategorizationRenderer.tsx
Outdated
Show resolved
Hide resolved
packages/vanilla-renderers/src/complex/categorization/CategorizationRenderer.tsx
Outdated
Show resolved
Hide resolved
8500538 to
bfcb670
Compare
lucas-koehler
left a comment
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 the updates. Looks better now :) I have two remarks inline.
Furthermore, the vanilla example app and some tests for the input control and the categorization renderer are failing.
I have a suspicion regarding the example app and input control: packages/vanilla-renderers/src/util/renderer.ts being in the util folder creates a circular dependency. Thus, renderer.ts should be moved up one folder to be directly contained in the src folder.
Please also have a look at the categorization renderer tests.
packages/vanilla-renderers/src/complex/categorization/CategorizationRenderer.tsx
Outdated
Show resolved
Hide resolved
packages/vanilla-renderers/src/complex/categorization/CategorizationList.tsx
Outdated
Show resolved
Hide resolved
|
@TheZoker Thanks for the updates :) |
sdirix
left a comment
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! Can you squash this into 1-2 nicely formatted commits?
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.
This file should be called renderers as we typically use plurals.
| VerticalLayout, | ||
| verticalLayoutTester, | ||
| } from './layouts'; | ||
| import DateTimeCell from './cells/DateTimeCell'; |
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.
Can you align this import with the others?
This should fix #2097