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

Add pagination.stackAllItems option #1214

Merged
merged 1 commit into from
May 2, 2020

Conversation

szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented May 2, 2020

Moved from #1204
Fixes #1200

Had to open this PR because git was complaining that the @PopGoesTheWza branch was in middle of merging even though it clearly was not. Dunno what happend.

/cc @PopGoesTheWza

@PopGoesTheWza
Copy link
Contributor

@szmarczak the fuss with previous PR is likely due to me wrongfully pull from upstream/master two days ago

@PopGoesTheWza
Copy link
Contributor

@sindresorhus @szmarczak I would humbly suggest the following 2 breaking changes:

  • Switch order of allItems and currentItems parameters
  • Default value for stackAllItem to false

My reasoning about parameters order is that (hopefully/probably) most use cases don't need the allItems parameters, with currentItems typically being more used. As per the default, the computation of allItems when unused implies a waste of CPU and RAM resources (and a crash in worst case scenarios)

Anyhow, let me know if/how I can assist more on this PR.

Regards,

@sindresorhus
Copy link
Owner

We cannot do breaking changes for a while, but open a new issue and we’ll consider it for the next major release.

@PopGoesTheWza
Copy link
Contributor

Sure. I will open an enhancement request after stackAllItems option is released.

@szmarczak
Copy link
Collaborator Author

Switch order of allItems and currentItems parameters

I don't think there's a need to. It doesn't change how the code works.

Default value for stackAllItem to false

I don't have a particular opinion on this one. I'm good with either.

@szmarczak szmarczak requested review from sindresorhus and removed request for sindresorhus May 2, 2020 11:26
@sindresorhus sindresorhus merged commit b2e6734 into sindresorhus:master May 2, 2020
sindresorhus pushed a commit that referenced this pull request May 2, 2020
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.

Add an option to disable stacking paginated items
3 participants