-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[UI v2] Adds Create or edit dialog for concurrency limit #16248
Conversation
e9d76c9
to
16eeace
Compare
...rency/global-concurrency-view/create-or-edit-limit-dialog/use-create-or-edit-limit-dialog.ts
Outdated
Show resolved
Hide resolved
const [openAddOrEditDialog, setOpenAddOrEditDialog] = | ||
useState<AddOrEditDialogState>({ | ||
open: false, | ||
}); |
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 had some trouble wrapping my head around this on the first couple of reads. This suggestion I think it a little more clear, but I'm open to alternatives.
const [openAddOrEditDialog, setOpenAddOrEditDialog] = | |
useState<AddOrEditDialogState>({ | |
open: false, | |
}); | |
const [addOrEditDialogState, addOrEditDialogState] = | |
useState<AddOrEditDialogState>({ | |
open: false, | |
limitIdToEdit: 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.
Based on your comment below here: https://github.com/PrefectHQ/prefect/pull/16248/files#r1873864935, this makes sense.
I was trying to represent the dialog state to:
- is it open?
- Should it open with a limit to be edited?
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.
Updated in latest push.
Simplified it to it just keeps track of it being open.
I removed the selected row logic and will handle it when doing the table
const selectedlimitToUpdate = useMemo(() => { | ||
if (!openAddOrEditDialog.limitIdToEdit) { | ||
return undefined; | ||
} | ||
return data.find((limit) => limit.id === openAddOrEditDialog.limitIdToEdit); | ||
}, [data, openAddOrEditDialog.limitIdToEdit]); |
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.
We might want to revisit this once the table is created. We should be able to have the table put the full data in a callback, which would negate the need to have a .find
call.
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'll keep an eye out for it, because this approach is to help eliminate duplicate state - which can lead to UX bugs.
eg problem case for duplicating state:
- UI gets list of data from
useSuspenseQuery
. - UI duplicates selected data into a
useState
. useSuspenseQuery
refetches, and updates its state of that selected data. (from some async event). The UI is now working with stale data.
When using this memoize approach.
- UI gets list of data from
useSuspenseQuery
. - UI keeps track of the selected data by id via
useState
. - Selected data is found by
.find()
. useSuspenseQuery
refetches, and updates its state of that selected data. (from some async event).- the selected data that calls
.find()
is ran because theuseMemo
dependency has changed from the async event. The UI is now working with the updated data source
Here's a good blog that goes in detail about it: https://blog.webdevsimplified.com/2019-11/never-store-derived-state/
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.
That makes sense. I don't think we'll need to worry about stale data from the table because we can directly pass in the data from our query to useTable
and retrieve the original data for the row to avoid working with duplicated data.
Plus, since we're using this data in a form, I don't think we want it to change if a new update comes in since that means the form values could change while the user is editing a concurrency limit.
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 nice, that makes sense
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'll update that when I implement the table next
065a21a
to
1033e3d
Compare
1033e3d
to
ad1cf8e
Compare
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.
LGTM!
cli
Will add tests when testing for the entire page functionality. Similar to
variables.test.tsx
. I think our hook tests inhooks/global-concurrent-limits.test.tsx
is suffice for now for confidenceChecklist
<link to issue>
"mint.json
.Related to #15512