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.

Sign up for GitHub

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

Jump to bottom

Feat: add size listing parameter #881

Merged
merged 5 commits into from
Feb 5, 2025
Merged

Feat: add size listing parameter #881

merged 5 commits into from
Feb 5, 2025

Conversation

Francisco2002
Copy link
Collaborator

Add listingSize parameter to tree and hardware listing. The alias to this param is ls.

How to test

  1. Go to tree listing or hardware listing
  2. On the URL, append the ls parameter
  3. See that the page number is the next decade. If you give ls=13, the page size will be 20. The max page size is 50.

Close #860

@Francisco2002
feat: add listingSize search params
6ed72da 
- add the new parameter to Search Keys
- add minified param
- create default value to listingSize

Part of #860
@Francisco2002
cleanup: remove redundant definiton of constant
565d7a5 
- use DEFAULT_TIME_SEARCH from types/general
@Francisco2002 Francisco2002 force-pushed the feat/add-size-listing-parameter branch from d09dc2c to 8a40489 Compare February 4, 2025 18:47
murilx
murilx reviewed Feb 4, 2025
View reviewed changes
Copy link
Contributor

@murilx murilx left a comment

Choose a reason for hiding this comment

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

Seems to be working fine, just some minor comments

dashboard/src/components/TreeListingPage/TreeTable.tsx Outdated Show resolved Hide resolved
dashboard/src/hooks/usePaginationState.tsx Outdated Show resolved Hide resolved
dashboard/src/pages/Hardware/HardwareTable.tsx Outdated
Comment on lines 404 to 408
navigate({
search: s => ({ ...s, listingSize: pageSize }),
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about state

dashboard/src/hooks/usePaginationState.tsx Outdated Show resolved Hide resolved
MarceloRobert
MarceloRobert reviewed Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

There seems to be a problem when you try to assign a negative number to the ls parameter, the search parameter goes back to the default but the table doesn't

dashboard/src/hooks/usePaginationState.tsx Outdated Show resolved Hide resolved
dashboard/src/routes/(alternatives)/t/route.tsx Outdated Show resolved Hide resolved
@Francisco2002 Francisco2002 self-assigned this Feb 5, 2025
@Francisco2002 Francisco2002 force-pushed the feat/add-size-listing-parameter branch from 8a40489 to 66655e2 Compare February 5, 2025 14:25
murilx
murilx reviewed Feb 5, 2025
View reviewed changes
Copy link
Contributor

@murilx murilx left a comment

Choose a reason for hiding this comment

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

Seems to be working fine, there's just some minor improvements to do

dashboard/src/hooks/usePaginationState.tsx Outdated Show resolved Hide resolved
dashboard/src/types/general.ts Outdated Show resolved Hide resolved
@Francisco2002
feat: add listingSize to router params
d93c315 
- add to tree listing (and alias) and hardware listing routes
- define type params as number

Part of #860
@Francisco2002 Francisco2002 force-pushed the feat/add-size-listing-parameter branch from 66655e2 to 72c1293 Compare February 5, 2025 16:52
MarceloRobert
MarceloRobert approved these changes Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

Worked well on my tests

dashboard/src/hooks/usePaginationState.tsx Outdated
Comment on lines 11 to 13
const DEFAULT_PAGINATION_STATE = { pageIndex: 0, pageSize: 10 } as const;

const DECADE = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: You could use DEFAULT_LISTING_SIZE here

Copy link
Contributor

Choose a reason for hiding this comment

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

Or something like TABLE_SIZE_STEP I'd say would work as well. But I agree that DECADE is not very descriptive

murilx
murilx approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@murilx murilx left a comment

Choose a reason for hiding this comment

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

Nice job!

@Francisco2002
feat: improve pagination options
f789835 
- change usePaginationState hook to receive a defaultValue of pageSize
- change PaginationInfo component to receive a paginationChange action

Part of #860
@Francisco2002
feat: use listingSize param in tree and hardware
379e97a 
- receive param from url and pass to Pagination component
- create function to navigate with the selected pageSize

Close #860
@Francisco2002 Francisco2002 force-pushed the feat/add-size-listing-parameter branch from 72c1293 to 379e97a Compare February 5, 2025 17:31
@Francisco2002 Francisco2002 merged commit c793f4d into main Feb 5, 2025
5 checks passed
@Francisco2002 Francisco2002 deleted the feat/add-size-listing-parameter branch February 5, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murilx murilx murilx approved these changes

@MarceloRobert MarceloRobert MarceloRobert approved these changes

Assignees

@Francisco2002 Francisco2002

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add GET parameter for size of the tree/hardware list
3 participants
@Francisco2002 @murilx @MarceloRobert