-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pagination Hook #8
Conversation
@bakerac4 the "hook" type concept we talked about seems to work pretty well, take a look at this when you get a chance. I have it mostly done, I just need to fully implement filtering, some better typings and a few other odds and ends, but nothing that should cause any issues I don't think. The main secret sauce that got this to come together was using Check out the |
(leaving these notes here, partially so i remember myself) It dawned on me as I was working on this, what this "hook" is really doing is mimicking a Controller > child Component relationship, without the Component (the "child" is just the property on the Controller that contains the paginator instance). And post-octane, the recommended/happy path for doing this kind of things (creating reusable/composable logic) w/o mixin is using Components. Especially now that Glimmer Components don't have a wrapper/root element by default to pollute the DOM with, for "logic only" type components/functionality like this. With that in mind, I'm thinking we keep this hook pattern like it is now, for easier backwards-compat w/our old patterns, and for cases where for some reason you really need the pagination instance on the controller itself, but I'll also add a component wrapper for it, which would be the recommended path moving forward. It would reuse the same paginator hook/class, but abstracts it away even further, making things even cleaner. In this case you wouldn't even need to touch the Route/ <Pagination
@modelName="customer"
@initialRows={{this.model}}
@initialMetadata={{this.model.meta}}
@limit={{50}}
@filters={{hash keyword=this.keyword}}
as |paginator|
>
<span>{{paginator.metadata.totalCount}} results</span>
<ButtonSpinner
@label="filter"
@action={{paginator.filterModels}}
/>
<Table
@rows={{paginator.rows}}
@hasMoreRows={{paginator.hasMore}}
@loadMoreRows={{paginator.loadMoreModels}}
@isLoading={{paginator.isLoadingModels}}
/>
</Pagination> And this still allows the multiple paginator per page scenario just as easily: <Pagination @modelName="some-model" as |paginatorOne|>
{{!-- .... --}}
</Pagination>
<Pagination @modelName="another-model" as |paginatorTwo|>
{{!-- .... --}}
</Pagination>
{{!-- Or, if needed, even nested --}}
<Pagination @modelName="some-model" as |paginatorOne|>
<Pagination @modelName="another-model" as |paginatorTwo|>
{{!-- .... --}}
</Pagination>
</Pagination> This pattern actually even simplifies usage even more when filtering comes into play, especially w/url queryParam based filters, as they must exist as properties on the controller. The the "hook" based pattern, we'll have to expose a/function arg of some sort that the user manually defines in the controller and that gets called by the paginator when you want to filter, so that those props can be added to the filter params object. With the component based pattern, you'd simply pass them in w/everything else as an |
@billdami the main reason I think we need non-component based pagination is to handle loading route states and to ensure that the data is loaded before we show the page. |
@bakerac4 that still works with the component-based version. You are still fetching the "initial page" of results in the route, that stays the same. If you look at the dummy app's list route in this branch, basically all this would remove is the That being said, I'm content with just doing the controller-based hook pattern for the first release, and then adding a component pattern later if it works out well, as it would be a purely additive and backwards-compatible change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall - Im glad what we discussed worked out so well and I think this is the way forward. Types overall look solid - Id recommend doing a ts-precompile to see if there are any other issues though as sometimes there are
addon/hooks/pagination.ts
Outdated
export interface PaginationArgs<T extends DS.Model, M = ResponseMetadata> extends PaginationConfigs { | ||
context: any; | ||
modelName: string; | ||
rows: NativeArray<T> | T[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billdami not sure if I love the rows
name. Thats more table-centric and half the time this wont be used in a table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm on the fence. I don't totally agree that "rows" is table-centric, as all lists have "rows", and what do you paginate in a UI if not rows? That being said, I don't care too much about it, and could possibly be convinced to go with something more ui-generic, such as items
or models
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are paginating a horizontally scrollable list you have columns
so I would prefer items
or models
but that's just my 2 cents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They would still be rows if you just turned your head sideways when looking at them. 😄
But yea thats a fair point. I'm fine with changing it to models
as it more aligns with the modelName
arg, and i think items
maybe a bit too generic.
addon/hooks/pagination.ts
Outdated
return this.loadModels(); | ||
} | ||
|
||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billdami is there a reason we are returning null
here instead of undefined (and just having the if
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its because of TS's "Not all code paths return a value." error you get otherwise, even if you explicitly define the return type of this method here as Promise<T[]> | undefined
, which I think is a bit weird.
Do you know of a better way to suppress those warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we positive its TS or is it es-lint? Either way, I wonder if it would work if you just did return undefined
at the bottom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its TS, this is the full warning text:
Not all code paths return a value. ts(7030)
But yup, just switching it to return undefined
works.
addon/utils/query-params.ts
Outdated
sorts = [], | ||
sortKey = 'sort', | ||
serverDateFormat = 'YYYY-MM-DDTHH:mm:ss' | ||
}: getParamsObjectArgs) { | ||
let params: any = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we could probably type this a bit better - at the very least so its an object that takes any string key with a value
No description provided.