area17 / twill

Twill is an open source CMS toolkit for Laravel that helps developers rapidly create a custom admin console that is intuitive, powerful and flexible. Chat with us on Discord at https://discord.gg/cnWk7EFv8R.
https://twillcms.com
Apache License 2.0
3.72k stars 568 forks source link

Nested repeater fields returned as blocks #1060

Open quotidianvoid opened 3 years ago

quotidianvoid commented 3 years ago

When using repeater fields outside of the block editor, the first level of repeaters are properly returned as repeaters, but second level repeaters are returned as blocks. This causes methods such as getFormFieldsForRepeater and updateRepeater to fail when implemented on the intermediary class.

When using nested repeaters, child objects should be treated as repeaters, not as blocks.

pboivin commented 3 years ago

Hi @quotidianvoid, I can confirm this issue. Thanks for reporting!

I could be wrong, but I believe nested repeaters were implemented initially with a focus on use cases inside the block editor. I agree with you that the use case of standalone repeaters could be improved.

If you are looking for a quick solution, I think you could use the HandleJsonRepeaters behavior to save your nested repeater content in a JSON column, instead of a dedicated table.

Here's a quick tutorial on HandleJsonRepeaters: https://gist.github.com/mcylinder/6ff7876f29cce8a23b81f3ea80e02a23

Hope this can help!

venkataadithan commented 3 years ago

Hi @pboi20, you are right.

Nested jsonRepeaters are good to save in this json column, But the values are not shown properly in the nested repeater.

`@twillRepeaterTitle('Custom Option') @twillRepeaterTrigger('Add Custom Option') @twillRepeaterGroup('app')

@formField('input', [ 'name' => 'custom_option', 'label' => 'Custom Option', 'required' => true ])

@formField('repeater',['type'=>'custom_values'])`

Here is the example of my code, First repeater "Custom Option" works good, but the nested/second repeater custom_values failed to show the given value after page reload.

pboivin commented 3 years ago

Hey @venkataadithan, thanks for the feedback!

I did some more testing and I can confirm this — nested JSON repeaters are also affected by the same issue. After some tinkering, I was able to make it work by reusing getJsonRepeater().

Context:

app/Repositories/TeamMemberRepository.php :

class TeamMemberRepository extends ModuleRepository
{
    use HandleTranslations, HandleSlugs, HandleJsonRepeaters;

    // Empty array because we only want to access `getJsonRepeater()` from the trait
    protected $jsonRepeaters = [];

    public function __construct(TeamMember $model)
    {
        $this->model = $model;
    }

    public function afterSave($object, $fields)
    {
        // Save the child repeater content to a JSON field
        $object->skill = collect($fields['blocks']['skill'] ?? [])->pluck('content');
        $object->save();

        parent::afterSave($object, $fields);
    }

    public function getFormFields($object)
    {
        $fields = parent::getFormFields($object);

        // Use `getJsonRepeater()` to load the content from the JSON field
        if ($object->skill) {
            $fields = $this->getJsonRepeater($fields, 'skill', $object->skill);
            $fields['repeaterMedias'] = ['skill' => []];
            $fields['repeaterFiles'] = ['skill' => []];
        }

        return $fields;
    }
}

Functional... but obviously not very robust. If anything, it's just a way to reiterate the limitations of repeaters at this point in time.

venkataadithan commented 3 years ago

Hey @pboi20 I have tried this solution but not worked, In my case as per your example i'm having Teammember as a repeater and with in that repeater i have skill repeater.

Does this get fixed in future?

Thanks for you help :)

venkataadithan commented 3 years ago

Hey @pboi20, This workaround works as expected!! Thanks.

A small change in that code to save nested repeater as having own id to prevent conflict within the nested repeaters.

class TeamMemberRepository extends ModuleRepository
{
    use HandleTranslations, HandleSlugs, HandleJsonRepeaters;

    // Empty array because we only want to access `getJsonRepeater()` from the trait
    protected $jsonRepeaters = [];

    public function __construct(TeamMember $model)
    {
        $this->model = $model;
    }

    public function afterSave($object, $fields)
    {
        // Save the child repeater content to a JSON field
        $object->skill = collect($fields['blocks']['skill'] ?? [])->map(function($option){
            $arr = $option['content'];
            unset($option['content']);
            return array_merge($arr,$option);
        });
        $object->save();

        parent::afterSave($object, $fields);
    }

    public function getFormFields($object)
    {
        $fields = parent::getFormFields($object);

        // Use `getJsonRepeater()` to load the content from the JSON field
        if ($object->skill) {
            $fields = $this->getJsonRepeater($fields, 'skill', $object->skill);
            $fields['repeaterMedias'] = ['skill' => []];
            $fields['repeaterFiles'] = ['skill' => []];
        }

        return $fields;
    }
}

(Edited for syntax highlighting)

Tofandel commented 6 months ago

I'm having the same issue and I absolutely need n level nested inline repeaters, which currently are not supported

I'm willing to work on a PR, has any work on this been attempted already?

I believe the repeater could really be simplified and work in nested configuration no matter where, no matter what and json wouldn't even need a separate behavior to handle it

I'll tackle just fixing nested repeaters outside blocks for now but for v4 I'm willing to refactor some stuff in the way fields are stored and handled on the frontend to be more efficient and easier to work with

Tofandel commented 6 months ago

There is actually a bug right here https://github.com/area17/twill/blob/3.x/src/Repositories/Behaviors/HandleRepeaters.php#L616

+ $fields['repeaters']["blocks-$relation-{$relationItem->id}|$childRepeaterName"] = $childRepeaterItems;
- $fields['repeaters']["blocks-$relation-{$relationItem->id}_$childRepeaterName"] = $childRepeaterItems;

It's what's preventing two levels of repeater from finding the correct block of data, see https://github.com/area17/twill/blob/703308854bbef11ff2bf2d61406f00bda351f84c/frontend/js/mixins/block.js#L25

Then if we also compute the repeater name from multiple levels we can get data n level deep (then the only problem is performance it outputs a ton of blocks (like 1000 or 10 000 if you have 3 level deep with a lot of fields)