Conversation
|
atk
left a comment
atk
left a comment
There was a problem hiding this comment.
Please let's discuss the API. I dislike the boolean and that the params need to be wrapped in a function even if they are not supposed to change.
packages/pagination/src/index.ts
Outdated
|
|
||
| /** | ||
| * Provides an easy way to implement infinite scrolling. | ||
| * A SolidJS utility to create an infinite scrolling experience with IntersectionObserver and reactivity. |
There was a problem hiding this comment.
There is no need to explicitly mention SolidJS here when the whole package is part of solid-primitives.
packages/pagination/src/index.ts
Outdated
| * A function that takes a page number and optional parameters and returns a Promise resolving to an array of items. | ||
| * | ||
| * @param {Object} [options] - Optional configuration object. | ||
| * @param {() => P} [options.params] - A reactive accessor for dynamic fetcher parameters. |
There was a problem hiding this comment.
We usually use the pattern that we take a params object or a params object signal. If it is a signal, we expect that we should reset state on params change; otherwise, we don't.
|
@atk I have updated the api design. Please check once |
| export type _E = JSX.Element; | ||
|
|
||
| /** Function-shaped accessor with live .loading / .error properties */ | ||
| export type PagesAccessor<T> = (() => T[]) & { |
There was a problem hiding this comment.
This looks awefully similar to a Resource<T>. I don't think we should re-create Solid's types without a good reason.
There was a problem hiding this comment.
Yeah, that was my thought too, Solid’s types felt like a solid (no pun intended) starting point for this hook. Since the JS docs mentioned patterns like
pages.loading
pages.error
it made sense to align the structure with Solid’s Resource style for familiarity and consistency.
There was a problem hiding this comment.
Then why can't you use the original type?
3 participants
This PR fixes the issue: #795