-
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
Transform CategorizationRenderer from class to functional #2120
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. |
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
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
filteredCategories
prop is of the wrong type and does not considerCategorization
elements despite them effectively being handed down. Handing them down seems to be correct because otherwise we could no longer handle nestedCategorization
s. 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
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 :) |
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