feat(site): add stop and start batch actions #10565
Conversation
| <MoreVertOutlined /> | ||
| </IconButton> | ||
| ); | ||
| return cloneElement(children as ReactElement, { |
There was a problem hiding this comment.
Is this type-safe? My worry is that it looks like there's nothing restricting the type of children at the type level, so if someone accidentally passes in a string or a primitive as a prop, they would get a more cryptic runtime error
There was a problem hiding this comment.
I swear I tried my best to get a correct type for this without success :(
| @@ -92,12 +87,18 @@ const WorkspacesPage: FC = () => { | |||
| }, [experimentEnabled, data, filterProps.filter.query]); | |||
| const updateWorkspace = useWorkspaceUpdate(queryKey); | |||
| const [checkedWorkspaces, setCheckedWorkspaces] = useState<Workspace[]>([]); | |||
| const [isDeletingAll, setIsDeletingAll] = useState(false); | |||
| const [isConfirmingDeleteAll, setIsConfirmingDeleteAll] = useState(false); | |||
| const [urlSearchParams] = searchParamsResult; | ||
| const { entitlements } = useDashboard(); | ||
| const canCheckWorkspaces = | ||
| entitlements.features["workspace_batch_actions"].enabled; | ||
| const permissions = usePermissions(); | ||
| const batchActions = useBatchActions({ | ||
| onSuccess: async () => { | ||
| await refetch(); |
There was a problem hiding this comment.
Does this need to be awaited? I'm wondering if we could just refetch without await, so the state can be cleared out faster
There was a problem hiding this comment.
Yes, it is needed because we only want to remove the loading state from this action when the new info is on the screen.
| @@ -92,12 +87,18 @@ const WorkspacesPage: FC = () => { | |||
| }, [experimentEnabled, data, filterProps.filter.query]); | |||
| const updateWorkspace = useWorkspaceUpdate(queryKey); | |||
| const [checkedWorkspaces, setCheckedWorkspaces] = useState<Workspace[]>([]); | |||
There was a problem hiding this comment.
Something I've been wondering about for a little bit: does it make sense to copy the checked workspaces into a separate piece of state, or could this be replaced with a Set object containing the IDs of workspaces that have been checked? That way, the original array and the Set could be cross-referenced
I know the current implementation simplifies the setup for calling the batch action functions, but I guess I've just been worried that with state values that lean more redundant, there might be bugs where they get out of sync down the line
There was a problem hiding this comment.
I think so, I just don't see this being used very often. Would you have any extra resources sharing the benefits of this approach?
There was a problem hiding this comment.
So, there's a really good example I can point to, but sadly, it was from Josh Comeau's Joy of React course, and that's pricey
A lot of the new official React documentation makes a lot of references to avoiding duplicate state, though. This section touches on it a little bit
There was a problem hiding this comment.
I see, so instead of objects we could use ids, is that right?
There was a problem hiding this comment.
Right, maybe a Set isn't the right data structure, but the idea is that it would have nothing but IDs in it
| @@ -345,8 +345,8 @@ dark = createTheme(dark, { | |||
| root: { | |||
| // It should be the same as the menu padding | |||
| "& .MuiDivider-root": { | |||
| marginTop: 4, | |||
| marginBottom: 4, | |||
| marginTop: `4px !important`, | |||
There was a problem hiding this comment.
Is this solving a bug, or is this more just a precaution?
I'm just worried about !important introducing specificity wars if any components ever need to override this
There was a problem hiding this comment.
Solving an issue. In this case, MUI is doing a very long chain to apply this style and I was not able to figure out how to make that happen here so I just used !important even not proud of that 😞
| import { MONOSPACE_FONT_FAMILY } from "theme/constants"; | ||
|
|
||
| export const useBatchActions = (options: { | ||
| onSuccess: () => Promise<void>; |
There was a problem hiding this comment.
I think this should be typed as () => (void | Promise<void>), just so components are free to use sync or async logic when it makes sense for them
There was a problem hiding this comment.
This component has a very specific use case and should only be used in one place so I would not try to make its interface generic to be re-used
| props: BatchDeleteConfirmationProps, | ||
| ) => { | ||
| const { checkedWorkspaces, open, onClose, onConfirm, isLoading } = props; | ||
| const [confirmation, setConfirmation] = useState({ value: "", error: false }); |
There was a problem hiding this comment.
What values is value expected to have right now? So far, it looks like "" or DELETE?
I'm wondering if value could have its own type, if we're only going to be supporting a discrete set of operations
There was a problem hiding this comment.
It is a good idea but it does not work well for "dynamic" values since it can be whatever the user types and not only the "final" result. Makes sense? The values can be "D", "DE", "DEL", "DELE" or anything like "OTHER".
|
Just a question from UX point of view. Is not the bottom left corner the default place for actions? |
I usually see it on the top right or left in a table when used to batch actions. 🤔 |
Demo:
Screen.Recording.2023-11-07.at.15.13.17.mov