Skip to content
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

Add HierarchyRepeater #1206

Merged
merged 22 commits into from May 1, 2022
Merged

Add HierarchyRepeater #1206

merged 22 commits into from May 1, 2022

Conversation

cafour
Copy link
Contributor

@cafour cafour commented Nov 5, 2021

There is currently no control in the framework able to render recursive hierarchies, so I backported HierarchyRepeater from business pack over here.

Implementing any ItemsControl, let alone a repeater, is a mine field, so any feedback is very, very welcome.

@cafour cafour requested a review from exyi November 5, 2021 16:52
Copy link
Contributor

@quigamdev quigamdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to rewrite this code to use the standard Item Template mechanism as Repeater uses.
Tbh we should completely rewrite this control.

@quigamdev quigamdev added this to the Version 5.0 milestone Nov 7, 2021
@cafour
Copy link
Contributor Author

cafour commented Nov 7, 2021

@quigamdev

We need to rewrite this code to use the standard Item Template mechanism as Repeater uses.

What do you mean by standard item template mechanism? This control already uses the same mechanism as Repeater with the exception of the ResourceManager.AddTemplateResource method.

This method cannot be used since it generates the template id based on the hash of template's contents... which needs to contain the template id. This can be overcome in two ways: either by rendering the template twice -- once to obtain the id and once to put the id back in, or by generating the template id in some other way (as I did in this PR).

Tbh we should completely rewrite this control.

This is already pretty much a complete rewrite of bp:HierarchyRepeater. I'd rather fix this implementation than start all over again.

@exyi exyi modified the milestones: Version 5.0, Version 4.1 Nov 7, 2021
@cafour cafour force-pushed the feature/hierarchy-repeater branch from aab8ece to 54d129c Compare April 22, 2022 10:09
@cafour cafour force-pushed the feature/hierarchy-repeater branch from 54d129c to 33c4a5c Compare April 29, 2022 11:12
@exyi exyi requested a review from quigamdev April 29, 2022 12:34
Copy link
Contributor

@quigamdev quigamdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. I have found just minor issues.
It would be nice to have also some perf test.

Copy link
Contributor

@quigamdev quigamdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just noticed that unit tests are failing.

@cafour
Copy link
Contributor Author

cafour commented Apr 29, 2022

Limitation: ItemWrapperCapability cannot have bindings with a data context set to the item. This limitation is cause by the need to have consistent date context nesting both in client-rendering and server-rendering.

@quigamdev quigamdev merged commit b7a1f47 into main May 1, 2022
@quigamdev quigamdev deleted the feature/hierarchy-repeater branch May 1, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants