Add pagination to manage automations #26414
Conversation
...end/pages/policies/ManagePoliciesPage/components/CalendarEventsModal/CalendarEventsModal.tsx
Show resolved
Hide resolved
...end/pages/policies/ManagePoliciesPage/components/CalendarEventsModal/CalendarEventsModal.tsx
Show resolved
Hide resolved
| {showPreviewCalendarEvent && ( | ||
| <CalendarEventPreviewModal | ||
| onCancel={togglePreviewCalendarEvent} | ||
| policy={selectedPolicyToPreview} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Moved the calendar preview to be a nested modal inside CalendarEventsModal so that the PoliciesPaginatedList can maintain its state (otherwise it loses the page its on whenever the preview is opened).
There was a problem hiding this comment.
We've generally been avoiding nested modals as a product rule. Would it be feasible to include the
lost state in PaginatedList's useImperativeHandle, and have the parent grab that state to
persist before rendering the preview modal, then pass it back in when it goes back to the paginated
list view?
If not, maybe just cosmetically hide the evidence of the parent modal when rendering the child? It
currently looks like:
| interface IFormPolicy { | ||
| name: string; | ||
| id: number; | ||
| installSoftwareEnabled: boolean; | ||
| swIdToInstall?: number; | ||
| platform: CommaSeparatedPlatformString; | ||
| } |
There was a problem hiding this comment.
This is now exported from PoliciesPaginatedList and shared with all the modals.
sgress454
changed the title
| // The size of the page to fetch and show. | ||
| pageSize?: number; | ||
| // The total # of items on all pages. | ||
| totalItems: number; |
There was a problem hiding this comment.
Where should this be used?
There was a problem hiding this comment.
That's TODO but I'd really like to update the Pagination control to accept it, because currently it just disables nextPage if the current # of page items is <= the size of a page, which means that if the last page has exactly numItemsOnPage items, we let you click "next" again and get an empty page:
There was a problem hiding this comment.
Actually I think I'll take this out for now, it's definitely worthwhile to do but this PR has got enough in it.
| } | ||
|
|
||
| // Wrap with forwardRef to expose the imperative handle: | ||
| const PaginatedList = forwardRef(PaginatedListInner) as <TItem>( |
There was a problem hiding this comment.
It looks like forwardRef will be deprecated soon,
is there a way to do this with the ref passed in as a prop instead?
There was a problem hiding this comment.
Ah I see...until we're on v19, you need to use forwardRef for the useImperativeHandle above. Can
we just leave a note then to change this whenever we upgrade to v19?
| }, | ||
| })); | ||
|
|
||
| // TODO -- better error state? |
|
Great work! A couple more things Looks like we lost name truncation on the calendar events modal: We lost a bit of form validation in the RunScript and InstallSoftware modals - in each of them, if any policies are selected but no script/software respectively is chosen for the selected policy, we want to disable the Save button: 
|
I have an ongoing discussion on this because I'm not sure it's a good idea when paginating, but I'll implement it as designed in the meantime.
Good catch, meant to wrap the labels in PaginatedList in |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26414 +/- ##
==========================================
+ Coverage 63.84% 63.85% +0.01%
==========================================
Files 1632 1638 +6
Lines 157954 158155 +201
Branches 4125 4173 +48
==========================================
+ Hits 100845 100993 +148
- Misses 49204 49254 +50
- Partials 7905 7908 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@jacobshandling updated this to:
--
|
|
Looks like we need to make the background colors line up for the nested modals: |
| @@ -36,10 +36,30 @@ | |||
| .form-field__help-text { | |||
| margin-top: $pad-large; | |||
| } | |||
|
|
|||
| &__hide-main { | |||
There was a problem hiding this comment.
Nice. I think you'll need to hide the app-wide overlay from the original modal as well, or something
like that.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Details
This PR updates the policy Manage Automations modals to support pagination. Previously, these modals received a list of policies from the main Manage Policies page, which is itself paginated, so that a user could only add automations to whatever policies were currently listed on the Manage Automations page. This PR does some refactoring via the creation of a new PaginatedList component which:
fetchPageproperty it can call to get a page of data,refFor this specific use case, there's also a new
PoliciesPaginatedListwhich implements thefetchPagefor getting a page of policies, and adds Save and Cancel buttons. Each of the updated modals usesPoliciesPaginatedListto replace its current code for rendering policies in a list, and delegates much of the logic around change tracking to the new components.Still TODO
totalItemsfor accurate pagination