refac: defer queries until the AssetActionMenu is actually opened#33078
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
SpencerWhitehead7
force-pushed
the
spencer/fe-1016-refactor-queries-that-run-before-asset-action-menu-is
branch
2 times, most recently
from
2036b4a to
3303704
Compare
hellendag
left a comment
hellendag
left a comment
There was a problem hiding this comment.
This looks good! Just a few more comments/requests inline.
| export const Popover = (props: BlueprintPopoverProps) => { | ||
| return ( | ||
| <Popover2 | ||
| <BlueprintPopover |
There was a problem hiding this comment.
Cool, these seem like fair changes here. Some of this is probably residual from refactoring from Popover to Popover2 ages ago, and it's appropriate to clean it up.
| type DeleteDynamicPartitionsMenuItemProps = { | ||
| assetKeys: AssetKeyInput[]; | ||
| locationName: string; | ||
| setIsShowing: Dispatch<SetStateAction<boolean>>; |
There was a problem hiding this comment.
This prop could be simpler, especially since it always just opens the dialog. e.g. onClick: () => void, and have the callsite use:
onClick={() => setIsShowing(true)}
And then you don't need the Dispatch and SetStateAction imports.
| refresh?: () => void, | ||
| ) { | ||
| const [showing, setShowing] = useState(false); | ||
| refresh?: VoidFunction, |
There was a problem hiding this comment.
I think these are clearer as () => void. (We're also not using VoidFunction elsewhere, plus it's undocumented.)
There was a problem hiding this comment.
I never realized VoidFunction only exists in dom types, I thought it was language level.
| /> | ||
| )) | ||
| : undefined} | ||
| <MenuDivider /> |
There was a problem hiding this comment.
I guess this could just be rendered if wipe.dropdownOptions.length || deletePartitions.dropdownOptions.length, so that way we're not rendering a rogue divider when there's nothing below it.
| type WipeDialogMenuItemProps = { | ||
| assetKeys: AssetKeyInput[]; | ||
| locationName: string; | ||
| setIsShowing: Dispatch<SetStateAction<boolean>>; |
There was a problem hiding this comment.
Same comment as above re: this handler.
SpencerWhitehead7
force-pushed
the
spencer/fe-1016-refactor-queries-that-run-before-asset-action-menu-is
branch
from
3303704 to
8acaa67
Compare
Merge activity
|
SpencerWhitehead7
deleted the
spencer/fe-1016-refactor-queries-that-run-before-asset-action-menu-is
branch
2 participants
Summary & Motivation
Hammer the backend less with
ASSETS_PERMISSIONS_QUERYsHow I Tested These Changes
Checked visually and logged the rendering behavior of the relevant components with console.log