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

Fix: nested repeaters #2490

Open
wants to merge 10 commits into
base: 3.x
Choose a base branch
from
Open

Conversation

Tofandel
Copy link
Contributor

@Tofandel Tofandel commented Feb 23, 2024

Fixes #1060

This allows Repeaters and InlineRepeaters to be nested to infinity (or until memory is exhausted at least, since they are memory hungry)

@Tofandel Tofandel force-pushed the fix-nested-repeaters branch 3 times, most recently from e8f84a1 to 029f666 Compare February 24, 2024 02:21
@ifox
Copy link
Member

ifox commented Feb 27, 2024

Hi @Tofandel, thank you so much for working on this!

The only thing I'm wondering about is whether or not this change may have a breaking impact on existing revisions.

@Tofandel
Copy link
Contributor Author

Tofandel commented Feb 27, 2024

I am currently blocking on the fact that block editors inside nested repeaters don't play well they seem to be saving as an array inside of blocks[blockName][] instead of block[]

As for breaking change, given that previously nested repeaters had no way to work (only nested blocks where working) I don't think there would be one, I'm not familiar with how revisions are created is it directly from the frontend field list and not the dirty attributes of the model?

@Tofandel

This comment was marked as outdated.

@Tofandel

This comment was marked as outdated.

@Tofandel
Copy link
Contributor Author

Tofandel commented Feb 27, 2024

Okay I completely rechanged the approach, I got everything working perfectly finally, schema is clean and a bit less memory hungry than before

I also tested old revisions and switching backend while editing frontend since the structure is mostly the same and it's all backwards compatible, I made sure of it

@Tofandel Tofandel force-pushed the fix-nested-repeaters branch 8 times, most recently from c4f61d7 to 618b3fc Compare February 28, 2024 08:54
@Tofandel
Copy link
Contributor Author

Tofandel commented Feb 28, 2024

Good to go now repeaters are only stored like

parts
blocks-parts-1|lessons
blocks-lessons-1|content

Instead of the original approach of

parts
parts|blocks-parts-1|lessons
parts|blocks-parts-1|lessons|blocks-lessons-1|content

Or the current not working approach in 3.x

parts
blocks-parts-1-lessons
blocks-parts-1-blocks-lessons-1-content

This is the same as how blocks are stored flattened and it seems to work just fine so I just thought let's reuse that

@Tofandel
Copy link
Contributor Author

Tofandel commented Feb 29, 2024

I think I'm pretty much done on the best way to approach this by causing the least amount of breaking change

Now the schema hasn't even changed except for the nested repeaters outside the block editor as mentionned in the previous comment which was something that was not working before so it can hardly be called a breaking change and only a fix

As for testing, I'm personnaly testing on a module with 4 levels of inline repeaters with blocks and nested blocks and browsers inside those repeaters and repeaters inside those blocks and I'm happy to report that everything is working perfectly
image

@antonioribeiro
Copy link
Member

This is great @Tofandel , thank you!

@ifox ifox added this to the Twill 3.4 milestone Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested repeater fields returned as blocks
3 participants