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 pagination when there are nested components that don't have pagination #6708

Merged
merged 5 commits into from
Sep 11, 2023

Conversation

Simoneu01
Copy link
Contributor

@Simoneu01 Simoneu01 commented Sep 7, 2023

When there is a nested component before the ->links() pagination it will result in an error
Property [$paginators] not found on component: [child]

Example code:

<div>
    <livewire:child />
    @foreach ($this->posts as $post)
         <h1 wire:key="post-{{ $post->id }}">{{ $post->title }}</h1>
    @endforeach
    {{ $this->posts->links() }}
</div>

When the nested component is after the pagination it will give no error:

<div>
    @foreach ($this->posts as $post)
         <h1 wire:key="post-{{ $post->id }}">{{ $post->title }}</h1>
    @endforeach
    {{ $this->posts->links() }}
    <livewire:child />
</div>

A workaround to fix this issue is to add use WithPagination; also on child component, but then it will result an error with the pagination itself

P.S: I have this error in a component that was working with v2

@joshhanley joshhanley changed the title [V3] Failing test for pagination with nested component Failing test for pagination with nested component Sep 8, 2023
@mansoorkhan96
Copy link

Same issue here.

@joshhanley
Copy link
Member

@Simoneu01 thanks for the PR! In future, please don't use your main branch, create a separate branch with your fix 🙂

I've fixed the issue, but I believe there could be a better implementation.

Basically what was happening was the SupportPagination component hook was running for all components (including the child component that didn't have WithPagination. What this meant, was the paginator page resolvers were being set up for the child component when they shouldn't have been.

So to fix this, in any of the component hook lifecycle methods (boot and destroy in this instance), I've done a check if the component doesn't implement WithPagination and if it doesn't it returns early, so none of the functionality runs.

@calebporzio I think this could be made smoother with a static method that determines whether the hook should even be initialised or not, that gets called in ComponentHookRegistry::initalizeHook() method below before the hook is instantiated, which returns early and doesn't set the hook up if it returns false. The benefit of this would be the check doesn't then need to be added to all the hook lifecycle methods.

static public function initializeHook($hook, $target)
{
if (! isset(static::$components[$target])) static::$components[$target] = [];
static::$components[$target][] = $hook = new $hook;
$hook->setComponent($target);
return tap($hook)->setComponent($target);
}

If you'd prefer this, let me know and I'll update this PR before it's merged. Otherwise, this PR is good to go as it is.

Hope this helps!

@joshhanley joshhanley changed the title Failing test for pagination with nested component Fix pagination when there are nested components that don't have pagination Sep 9, 2023
@Simoneu01
Copy link
Contributor Author

Simoneu01 commented Sep 9, 2023

@Simoneu01 thanks for the PR! In future, please don't use your main branch, create a separate branch with your fix 🙂
[...]
Hope this helps!

Thank you @joshhanley, will do.

@calebporzio
Copy link
Collaborator

Thanks for the fix @joshhanley - yeah it makes total sense to add a dedicated API to only use a hook on a component when a condition is met.

I don't know exactly what that API is, though...

If you have an intuitive idea, feel free to implement it on this PR and we can merge it or tweak it till we're happy.

If you'd rather, you can merge this and do that in a separate PR. Up to you. Thanks!

@joshhanley
Copy link
Member

@calebporzio thanks! I will just merge this for now, and submit a new PR with the proposed changes.

@joshhanley joshhanley merged commit c1aa042 into livewire:main Sep 11, 2023
2 checks passed
@joshhanley
Copy link
Member

Have created a new PR #6774 for the conditional hook.

luttje added a commit to luttje/filament-user-attributes that referenced this pull request Nov 6, 2023
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

4 participants