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

Provide support for snapshot() function in PagingDataEpoxyController #1144

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Provide support for snapshot() function in PagingDataEpoxyController #1144

merged 1 commit into from
Mar 23, 2021

Conversation

anhanh11001
Copy link
Contributor

@anhanh11001 anhanh11001 commented Mar 3, 2021

The seems to be the only function in paging3 lib I can find to provide access to all loaded data. I think it's useful.

A use case can be that as a developer, I want to track all of the IDs of the loaded posts in a screen, then this one can be used.

@anhanh11001
Copy link
Contributor Author

anhanh11001 commented Mar 8, 2021

@elihart You mind checking this out. It's a small change.

I'm also wondering if you think it's necessary to:

  1. Should there be a paging3 epoxy sample?
  2. paging3 provides this function for the adapter https://developer.android.com/reference/kotlin/androidx/paging/PagingDataAdapter#withloadstateheaderandfooter which is convenient to add a loading indicator to the top/bottom of the list. Should there be something similar to the epoxy-paging3?

If you find them useful and have any suggestions, I can start looking into them as I have some free time to contribute.

Copy link
Contributor

@elihart elihart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anhanh11001 this change looks good to me, sorry for the delay in reviewing.

I think a paging3 sample could be helpful to people, so feel free to put up a PR if you are interested :) It could go in the same module as the paging2 sample https://github.com/airbnb/epoxy/tree/master/epoxy-pagingsample

As far as headers and footers, I think it is fairly easy for people to add those already by overriding the addModels function. While we could add functions explicitly to override to provide header or footer models to save them a few lines I don't think the extra complexity is worth the small savings and would rather leave the current API to maximize flexibility.

@anhanh11001
Copy link
Contributor Author

anhanh11001 commented Mar 21, 2021

I see. Thanks for replying.

As far as headers and footers, I think it is fairly easy for people to add those already by overriding the addModels function. While we could add functions explicitly to override to provide header or footer models to save them a few lines, I don't think the extra complexity is worth the small savings and would rather leave the current API to maximize flexibility.

Make sense to me

It could go in the same module as the paging2 sample https://github.com/airbnb/epoxy/tree/master/epoxy-pagingsample

Should I remove the paging2 sample because I don't think we can have 2 paging deps in the same module.?

@anhanh11001
Copy link
Contributor Author

Oh can you also add this to the next release

@elihart elihart merged commit d165a2c into airbnb:master Mar 23, 2021
@elihart
Copy link
Contributor

elihart commented Mar 23, 2021

Should I remove the paging2 sample because I don't think we can have 2 paging deps in the same module.?

ah, right. maybe it is best to create a new paging3 sample module for now then, since it doesn't have a stable release yet.

I'll try to make a new release with this change soon 👍

@elihart elihart mentioned this pull request Mar 24, 2021
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.

2 participants