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

[V2] Improve WhenVisible component to merge data and params props for a more flexible API #2048

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PedroAugustoRamalhoDuarte
Copy link

@PedroAugustoRamalhoDuarte PedroAugustoRamalhoDuarte commented Oct 20, 2024

This PR fixes #2025

Enhancements

  • Merge data and params props:
    This PR introduces a more flexible API that merges the data and params props. Now, you can pass both in a single configuration like this:

    <WhenVisible fallback="Loading..." data={["todos", "pagy"]} params={{
      data: {
        teste: true,
        page: pagy.page + 1,
      },
      preserveUrl: true,
    }} />

@PedroAugustoRamalhoDuarte
Copy link
Author

@joetannenbaum do you think this pr makes sense?

@joetannenbaum
Copy link
Contributor

Hey @PedroAugustoRamalhoDuarte, thanks for the PR. I'm not sure this makes sense to me to be honest.

The data prop is just shorthand for fetching specific data via the only property of params, if you include a data prop on the component and include a data property in params that seems confusing to me, I wouldn't quite understand what was going on by looking at it.

If it's only data, it indicates "this is the data I'd like". If it's only params, it indicates "I'd like to get more granular about this request," which can itself include an only property.

As a side note, I believe children are currently optional for the component, so that should already work.

@PedroAugustoRamalhoDuarte
Copy link
Author

PedroAugustoRamalhoDuarte commented Jan 9, 2025

Thanks @joetannenbaum for the response. The major issue that I am trying to solve is that now I cannot send query params with WhenVisible component, and that is very useful for example for pagination.

My ideia for solve this was to merge data and params props, but maybe we can think in other solution

@PedroAugustoRamalhoDuarte
Copy link
Author

Hey @PedroAugustoRamalhoDuarte, thanks for the PR. I'm not sure this makes sense to me to be honest.

The data prop is just shorthand for fetching specific data via the only property of params, if you include a data prop on the component and include a data property in params that seems confusing to me, I wouldn't quite understand what was going on by looking at it.

If it's only data, it indicates "this is the data I'd like". If it's only params, it indicates "I'd like to get more granular about this request," which can itself include an only property.

As a side note, I believe children are currently optional for the component, so that should already work.

Thanks for the side note, maybe other PR already made this fix!!

@joetannenbaum
Copy link
Contributor

Why can't you send query params with the component? If the method is get and you fill in the data property if the params prop, is that not working? It should send over query params with your request.

@PedroAugustoRamalhoDuarte
Copy link
Author

Yes, that’s exactly it. I think a better API would merge the two, as many people naturally expect that behavior is the past:

However, in the current code (modified in another PR after I created this one), I realized that the data prop is no longer necessary. We can now rely entirely on params, and it works as expected.

Thank you so much for taking the time to review this PR—I truly appreciate it!

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.

[V2] Improve WhenVisible component to merge data and params props for a more flexible API
2 participants