Transform CategorizationRenderer from class to functional by TheZoker · Pull Request #2120

Transform CategorizationRenderer from class to functional by TheZoker · Pull Request #2120 · eclipsesource/jsonforms

coverage: 84.796% (+0.009%) from 84.787%
when pulling a72b1e0 on TheZoker:categorization-to-functional
into 01b08f0 on eclipsesource:master.

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.

Can you check the failing test cases?

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 same filteredCategories. 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 consider Categorization elements despite them effectively being handed down. Handing them down seems to be correct because otherwise we could no longer handle nested Categorizations. The type and potentially the prop name should be fixed accordingly.

TheZoker and others added 9 commits

November 13, 2023 23:55

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.

@TheZoker Thanks for the updates :)
The test case Categorization renderer › reset selected category on schema change is still failing. Please have a look.

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?

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';

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?