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

[Feature] Add pagination hook method #3632

Merged
merged 4 commits into from
Sep 3, 2021

Conversation

joshhanley
Copy link
Member

@joshhanley joshhanley commented Aug 31, 2021

This PR adds a stub method to the pagination trait as a hook that users can override if they need any side effects to happen when pagination changes. For example, they may want to fire off a browser event, so they can tell the browser to scroll to the top on pagination changed.

Rather than Livewire firing off browser events that may not get used, this hook method instead lets the developer decide how they want to handle it.

I have named the method paginationPageChanged and it accepts the page the pagination has changed to as a property.

For example, this could be put into a component with pagination, to fire off a browser event.

public function paginationPageChanged($page) {
    $this->dispatchBrowserEvent('paginationChanged', $page);
}

I intentionally made it this method name to ensure the risk of a collision with an existing method name like this is minimal.

Another option would be to use the existing updating/updated convention and call it something like updatedPaginationPage.

Hope this helps!

@calebporzio
Copy link
Collaborator

@joshhanley - good addition. I like you're suggestion about using the existing "updating/updated" hooks.

Can we just call those? Like when we set the page, can we just pipe those calls through the standard "syncInput" so that those hooks are called automatically and people can just hook into them?

What do you think?

@joshhanley
Copy link
Member Author

@calebporzio Yep, I think those names are better.

Can't pump it through the standard syncinput the way it is structured at the moment though, because they get updated through action calls, not property updates.

I can just check if method exists and manually call updatingPage and updatedPage, which is kinda handy as you can get before and after state if you need it.

Then for the multiple paginators PR, there will be an array $paginators = [] for all the paginator instance names, so I could then call updatingPaginators and updatedPaginators any time any of them changes, but also call the specific one that actually got updated, updatingPaginatorsUserPage and updatedPaginatorsUserPage or something like that.

The question then is, do you want this existing solution for the single page paginator we currently have to call updatingPage/updatedPage or use the new standard and call updatingPaginatorsPage/updatedPaginatorsPage?

@calebporzio
Copy link
Collaborator

I think we actually can. Check this out:
image

By calling syncInput(...) we are automatically firing all hooks and using all the internal machinery for syncing a property.

I changed the tests and removed your hook - what do you think? I think this is a super elegant solution - solid thinking here.

@joshhanley
Copy link
Member Author

Yep that's nice!

@joshhanley joshhanley merged commit 396c5c9 into livewire:master Sep 3, 2021
@joshhanley joshhanley deleted the add-pagination-hook-method branch September 3, 2021 20:51
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.

None yet

2 participants