- Notifications
You must be signed in to change notification settings - Fork928
feat: paginate workspaces page#4647
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| severity={ | ||
| workspaceRefs !== undefined && workspaceRefs.length > 0 | ||
| ? "warning" | ||
| : "error" |
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.
What does theAlertBanner look like with the error prop passed through, but also a severity ofwarning? JC because I only ever tested the warning case with the text prop, as opposed to the error prop.
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.
Just added a story for this!
| @@ -206,160 +206,255 @@ interface WorkspacesContext { | |||
| workspaceRefs?: WorkspaceItemMachineRef[] | |||
| filter: string | |||
| getWorkspacesError?: Error | unknown | |||
| getCountError?: Error | unknown | |||
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.
Could these additions be pulled out into a separate machine for pagination? Not sure if there's a way to call a machine state from another machine.
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.
Maybe viaspawn? I saw that being used below.
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, that's the idea I mentioned!
One way would be for the pagination widget to have its own machine. I believe both machines would need to be aware of the other, so we'd have to hard-code in their machine ids. I wasn't sure I liked that. I think someday XState will make stuff like that easier.
Another way would be to have a pagination machine that doesn't operate on its own, but the workspaces page invokes it. I didn't go that route only because the pagination machine would have no states, and I didn't think people would want an extra machine for something simple.
But if you're in favor, I can reconsider.
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.
Oh, I see! I forgot about the machine IDs.
I do like the idea of a pagination machine. Otherwise, I think we'll have to duplicate the logic you've written here to each page machine.
| getWorkspacesError: (_, event) => event.data, | ||
| }), | ||
| clearGetWorkspacesError: (context) => | ||
| assign({ ...context, getWorkspacesError: undefined }), |
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.
curious why we overwrite the the whole context here instead of just updating thegetWorkspaceError to be undefined.
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.
Oh so something not great about this diff is that I used the XState editor and it rearranged some code, so some stuff looks new that isn't. This is just an old pattern that we found a better way of writing since then.
Kira-Pilot commentedOct 19, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The empty state is a bit off - neither buttons are disabled, and when clicked, I get this error: Also, I'm surprised to see the widget for 0 entries, but if there are less than 25 entries, I don't see it. |
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.
Great job! I do think we should make a pagination machine and also explore showing the pagination widget at all times, even if there is only 1 page.
But not gonna block this sweet PR over it! 🥳
@Kira-Pilot some of the weirdness is because I got rid of conditions in the page that block the widget, but I didn't change the conditions in the widget. Partly I'm trying to leave it for that other ticket and partly I'm afraid to change the widget before testing the audit log page. I think the widget enables the page turning buttons when count is undefined, so it's usable without a count, but doesn't distinguish between 0 and undefined, so in the meantime I'm not rendering it when count is 0. |
| q.mutex.RLock() | ||
| defer q.mutex.RUnlock() | ||
| workspaces := make([]database.Workspace, 0) |
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.
You could probably save some code by just converting this fromdatabase.GetWorkspaceCountParams todatabase.GetWorkspaceParams and then callingq.GetAuthorizedWorkspace but completely optional.
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 agree with this!
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.
Sorry for the late review!
| q.mutex.RLock() | ||
| defer q.mutex.RUnlock() | ||
| workspaces := make([]database.Workspace, 0) |
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 agree with this!
| -- this duplicates the filtering in GetWorkspaces | ||
| -- name: GetWorkspaceCount :one |
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 left an ad-hoc comment on the commit, but I think this is quite bug-prone.
It's not extremely obvious that youhave to change this when introducing a new filter type, which could lead to a mismatch with the workspaces and count.
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, Colin thought this was the best thing for now but we have an issue to clean it up#4604
The frontend test is not ideal but I wanted to get reviews before demo time!