"All Packages" table retreives all packages in the set, provides pagination and sorting#13
"All Packages" table retreives all packages in the set, provides pagination and sorting#13minghao912 wants to merge 3 commits intomasterfrom
Conversation
| } | ||
|
|
||
| fetchPackages = async (pages?: number, ascending?: boolean) => { | ||
| if (pages == undefined) |
|
|
||
| // Fetch new packages when either page or sorting order changes | ||
| useEffect(() => { | ||
| if (sortOrder == 'ascend') |
There was a problem hiding this comment.
Use === to avoid bugs. Change the rest as well
| @@ -41,11 +41,17 @@ export class ModelOperations { | |||
| async getPackages( | |||
There was a problem hiding this comment.
I'm still partial to adding a search feature since it's quite hard to find a package that has a specific name that's in the middle of the pages. It doesn't have to update the page dynamically, just make a request when the user presses "search"
| ]; | ||
|
|
||
| // Configure pagination | ||
| const paginationConfig = { |
There was a problem hiding this comment.
Could you add something that allows you to change the page size, like we have in gallery maker? This is something that's built into antd. I feel like it will make it easier to browse the table if it's really big
| ]; | ||
|
|
||
| // Configure pagination | ||
| const paginationConfig = { |
There was a problem hiding this comment.
I think pagination is a bit slow for my liking. The main problem is that there's no visual indication that the page is loading when you click a different number. We could go about this two different ways:
- Make pagination faster (maybe with caching or maybe optimizing the query)
- Give a visual indication that the page is loading when the user goes to a different page
| packageSetSize: this.state.totalPackagesInSet | ||
| } as PackageSetInfo; | ||
|
|
||
| {/* |
There was a problem hiding this comment.
Minor nitpick, but I don' think the braces need to surround the comment. Also, could you line break this block so it fits into one page? (Both the comment and the AllPackagesTable)
| } | ||
|
|
||
| export default Homepage; | ||
|
|
There was a problem hiding this comment.
It's not great to have this here, unless we're going to work this immediately after. If you don't foresee working on this anytime soon, we can move it into a new branch
| "dependencies": { | ||
| "@types/lodash": "^4.14.121", | ||
| "@types/luxon": "^1.26.5", | ||
| "@types/lodash": "^4.14.175", |
There was a problem hiding this comment.
Just curious why you updated the dependency versions? Nothing wrong with it just wondering if there was anything wrong with the previous versions
2 participants
No description provided.