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

wip: add support for multiple paginations in one component #1997

Closed
wants to merge 6 commits into from
Closed

wip: add support for multiple paginations in one component #1997

wants to merge 6 commits into from

Conversation

mokhosh
Copy link
Contributor

@mokhosh mokhosh commented Nov 9, 2020

1️⃣ Is this something that is wanted/needed? Did you create an issue / discussion about it first?
Yes, desperately. #1951

3️⃣ Does it include tests, if possible? (Not a deal-breaker, just a nice-to-have)
Nope, but can I interest you in some comments that describe how to finish up the work?

4️⃣ Please include a thorough description of the improvement and reasons why it's useful.
In laravel you can setPageName on a paginated query so you can have multiple paginations on one page without collision.
But Livewire doesn't respect the set name and hardcodes page for all paginated queries. So if you have two tabs for example, and in each tab you paginate over a different model, when you change page for one model, the other changes too.

This PR seeks to resolve this issue.

5️⃣ Thanks for contributing! 🙌
Thanks for such an awesome gift to humanity

@mokhosh
Copy link
Contributor Author

mokhosh commented Nov 9, 2020

The test are now failing because again $page is hardcoded in tests as well.
I'll have to change the tests if you don't prefer to do it yourself to be sure it's a valid test.

@calebporzio
Copy link
Collaborator

Thanks for submitting this @mokhosh - unfortunately, this is a breaking change (for people depending on $this->page being mutated by the pagination trait)

I think this should be supported, but this implementation would have to wait for the next major version.

I'm going to close this and reference this PR in the roadmap for the next version.

Thank you.

@calebporzio calebporzio closed this Nov 9, 2020
@mokhosh
Copy link
Contributor Author

mokhosh commented Nov 9, 2020

@calebporzio thanks for reviewing the PR. Are you sure it will break?

It doesn't use the array syntax for tracking pages. The array is just for initializing the actual public properties, and since there is a page default item, it shouldn't break anything.

That should actually be protected and is not meant to be used in the front.

@mokhosh
Copy link
Contributor Author

mokhosh commented Nov 10, 2020

@calebporzio I just did a proof of concept in my own project, and with a few changes the multi pagination awareness works.
Meaning that I can click on pagination links, and the querystring will get updated.

But I don't know how to tell the view to render that pages based on that pageName.

Also I think you will see that it's not breaking at all, because it still sets $page to 1 as default and uses 'page' as default for all methods.

P.S. The few changes that I mentioned was:

  1. use mapWithKeys instead of map
  2. pass $paginator->getPageName() in pagination view
  3. pass $pageName = 'page' as second parameter to goToPage too
    also don't forget to add your custom pagination names to the array, that's how the component knows about them and whitelists them.

@mokhosh
Copy link
Contributor Author

mokhosh commented Nov 10, 2020

REOPEN

@mokhosh
Copy link
Contributor Author

mokhosh commented Nov 10, 2020

Update: I got one step closer and I managed to change the page with custom pagination names too. I hardcoded my custom page name into Paginator::currentPageResolver, because it seems like it's being passed a hardcoded 'page' which I could not find to make it dynamic.

Also, I removed the WithPagination trait and this behaviour stopped working, so, it seems like there is some magic or something outside WithPagination that I couldn't find, or I'm just not familiar with how pagination works.

Please, I really want to help here, but I don't have enough guidance.

@calebporzio
Copy link
Collaborator

Thanks for fleshing this out further.

I'd be open to supporting multiple pagination if the implementation didn't break existing APIs, but it's not a high priority for me, so I'd be pretty picky.

Anyhow, this IS a breaking change because you've removed public $id; in the trait which means people who depend on or manipulate that in their component will be affected.

Also, you are using syntax (short closures) that isn't available in all livewire supported versions AND your tests are failing.

I'm closing this PR

@mokhosh
Copy link
Contributor Author

mokhosh commented Dec 23, 2020

@calebporzio Thank you for taking the time to explain in details, that helps me keep working on it. 👍

@Whallas
Copy link

Whallas commented Feb 11, 2021

I think the best way is to create a new trait for this purpose.

@mokhosh
Copy link
Contributor Author

mokhosh commented Feb 11, 2021

@Whallas why not use the same trait when it can be done without breaking anything?

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

4 participants