Skip to content
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.

Sign up for GitHub

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

Add FeedbackButton to DeleteDialog of CrudContextMenu #2237

Closed
wants to merge 8 commits into from
Closed

Add FeedbackButton to DeleteDialog of CrudContextMenu #2237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0d0d809
feat(CrudContextMenu): use FeedbackButton in error dialog, show snack…
jomunker Jun 12, 2024
97b6df8
docs: add changeset
jomunker Jun 12, 2024
46a1965
feat(CrudContextMenu): use tooltip instead of snackbar for status mes…
jomunker Jul 1, 2024
8226c9d
feat(FeedbackButton): rework to simplify usage by relying on Promises…
jomunker Jul 1, 2024
0c79e84
fix(FeedbackButton): linting of story
jomunker Jul 2, 2024
3ad0e3d
feat(FeedbackButton): re-add old way to handle the feedback state to …
jomunker Jul 31, 2024
34316b6
feat(FeedbackButton): make feedbackStateControl optional to ensure a …
jomunker Aug 1, 2024
4f4b410
feat(FeedbackButton): use never instead of undefined for props hasErr…
jomunker Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/metal-donuts-suffer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@comet/admin": minor
---

Use `FeedbackButton` in `DeleteDialog` of `CrudContextMenu`

This provides the user with feedback about the current status of the delete action.
79 changes: 58 additions & 21 deletions packages/admin/admin/src/common/buttons/feedback/FeedbackButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,41 @@ const Tooltip = createComponentSlot(CometTooltip)<FeedbackButtonClassKey>({
slotName: "tooltip",
})();

export interface FeedbackButtonProps
type FeedbackStateControl = "props" | "onClickPromise";

export interface FeedbackButtonPropsBase
extends ThemedComponentBaseProps<{
root: typeof LoadingButton;
tooltip: typeof CometTooltip;
}>,
LoadingButtonProps {
loading?: boolean;
hasErrors?: boolean;
Omit<LoadingButtonProps, "loading"> {
feedbackStateControl?: FeedbackStateControl;
startIcon?: React.ReactNode;
endIcon?: React.ReactNode;
tooltipSuccessMessage?: React.ReactNode;
tooltipErrorMessage?: React.ReactNode;
}

export interface FeedbackButtonProps extends FeedbackButtonPropsBase {
feedbackStateControl: "props";
onClick?: () => void;
hasErrors?: boolean;
loading?: boolean;
}

export interface FeedbackButtonPropsPromise extends FeedbackButtonPropsBase {
feedbackStateControl: "onClickPromise";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be inferred based on the passed onClick handler? Is it possible to detect if a function is async or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ways but it's not straightforward and probably also not reliable. Additionally, it is way more descriptive with the flag. So users will actually understand whats going on.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsams @thomasdax98 what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a solution that doesn't need this feedbackStateControl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. The other two options I can think of are:

  1. Somehow infer from function if it is async or not
  2. Add a second onClickAsync prop for the promise-based usage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's too late unfortunately. I could infer that a user wants to rely on outside state if loading and/or hasError is not undefined 🤷🏻‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's too late unfortunately. I could infer that a user wants to rely on outside state if loading and/or hasError is not undefined 🤷🏻‍♂️

I like this idea! Could you please try this? Also, I'd still recommend adding the promise detection in a separate PR: #2237 (review)

Copy link
Contributor Author

@jomunker jomunker Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new PR for the changes to CrudContextMenu: #2423
Was easier to do it like this since the current PR targets next. I'll add another one for the addition of the promise feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here to following PR: #2425

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will close this one

onClick: () => Promise<void>;
hasErrors?: never;
loading?: never;
}

type FeedbackButtonDisplayState = "idle" | "loading" | "success" | "error";

export function FeedbackButton(inProps: FeedbackButtonProps) {
export function FeedbackButton(inProps: FeedbackButtonProps | FeedbackButtonPropsPromise) {
const {
onClick,
feedbackStateControl = "props",
loading,
hasErrors,
children,
Expand All @@ -61,7 +78,6 @@ export function FeedbackButton(inProps: FeedbackButtonProps) {
tooltipSuccessMessage = <FormattedMessage id="comet.feedbackButton.tooltipSuccessMessage" defaultMessage="Success" />,
tooltipErrorMessage = <FormattedMessage id="comet.feedbackButton.tooltipErrorMessage" defaultMessage="Error" />,
slotProps,

...restProps
} = useThemeProps({
props: inProps,
Expand All @@ -71,29 +87,49 @@ export function FeedbackButton(inProps: FeedbackButtonProps) {
const [displayState, setDisplayState] = React.useState<FeedbackButtonDisplayState>("idle");

const ownerState: OwnerState = {
displayState: displayState,
displayState,
};

const resolveTooltipForDisplayState = (displayState: FeedbackButtonDisplayState) => {
if (displayState === "success") {
return "success";
} else if (displayState === "error") {
return "error";
} else {
return "neutral";
switch (displayState) {
case "error":
return "error";
case "success":
return "success";
default:
return "neutral";
}
};

const handleOnClick =
feedbackStateControl === "onClickPromise" && onClick
? async () => {
try {
setDisplayState("loading");
await onClick();
setDisplayState("success");
} catch (_) {
setDisplayState("error");
} finally {
setTimeout(() => {
setDisplayState("idle");
}, 3000);
}
}
: undefined;

React.useEffect(() => {
if (feedbackStateControl === "onClickPromise") return;

let timeoutId: number | undefined;
let timeoutDuration: number | undefined;
let newDisplayState: FeedbackButtonDisplayState;

if (displayState === "idle" && loading) {
setDisplayState("loading");
} else if (displayState === "loading" && hasErrors) {
timeoutDuration = 500;
newDisplayState = "loading";
timeoutDuration = 0;
newDisplayState = "error";
} else if (displayState === "loading" && !loading && !hasErrors) {
timeoutDuration = 500;
newDisplayState = "success";
Expand All @@ -115,27 +151,28 @@ export function FeedbackButton(inProps: FeedbackButtonProps) {
window.clearTimeout(timeoutId);
}
};
}, [displayState, loading, hasErrors]);
}, [displayState, loading, hasErrors, feedbackStateControl]);

const tooltip = (
<Tooltip
title={displayState === "error" ? tooltipErrorMessage : tooltipSuccessMessage}
title={displayState === "error" ? tooltipErrorMessage : displayState === "success" ? tooltipSuccessMessage : ""}
open={displayState === "error" || displayState === "success"}
placement={endIcon && !startIcon ? "top-end" : "top-start"}
variant={resolveTooltipForDisplayState(displayState)}
{...slotProps?.tooltip}
>
<span>{startIcon ? startIcon : endIcon}</span>
<span>{startIcon || endIcon}</span>
</Tooltip>
);

return (
<Root
onClick={handleOnClick}
ownerState={ownerState}
loading={loading}
loading={loading !== undefined ? loading : displayState === "loading"}
variant={variant}
color={color}
disabled={disabled || displayState === "loading"}
disabled={disabled || loading || displayState === "loading"}
loadingPosition={startIcon ? "start" : "end"}
loadingIndicator={<ThreeDotSaving />}
startIcon={startIcon && tooltip}
Expand All @@ -154,7 +191,7 @@ declare module "@mui/material/styles" {
}

interface ComponentsPropsList {
CometAdminFeedbackButton: FeedbackButtonProps;
CometAdminFeedbackButton: FeedbackButtonPropsBase;
}

interface Components {
Expand Down
38 changes: 22 additions & 16 deletions packages/admin/admin/src/dataGrid/CrudContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ import { FormattedMessage, useIntl } from "react-intl";

import { readClipboardText } from "../clipboard/readClipboardText";
import { writeClipboardText } from "../clipboard/writeClipboardText";
import { FeedbackButton } from "../common/buttons/feedback/FeedbackButton";
import { useErrorDialog } from "../error/errordialog/useErrorDialog";
import { messages } from "../messages";
import { RowActionsItem } from "../rowActions/RowActionsItem";
import { RowActionsMenu } from "../rowActions/RowActionsMenu";

interface DeleteDialogProps {
dialogOpen: boolean;
onDelete: () => void;
onDelete: () => Promise<void>;
onCancel: () => void;
}

Expand All @@ -29,12 +30,19 @@ const DeleteDialog: React.FC<DeleteDialogProps> = (props) => {
<FormattedMessage id="comet.table.deleteDialog.content" defaultMessage="WARNING: This cannot be undone!" />
</DialogContent>
<DialogActions>
<Button onClick={onDelete} color="primary">
<Button onClick={onCancel} color="primary">
<FormattedMessage {...messages.no} />
</Button>
<Button onClick={onCancel} color="primary" variant="contained">
<FeedbackButton
feedbackStateControl="onClickPromise"
startIcon={<DeleteIcon />}
onClick={onDelete}
color="primary"
variant="contained"
tooltipErrorMessage={<FormattedMessage id="comet.common.deleteFailed" defaultMessage="Failed to delete" />}
>
<FormattedMessage {...messages.yes} />
</Button>
</FeedbackButton>
</DialogActions>
</Dialog>
);
Expand All @@ -60,11 +68,15 @@ export function CrudContextMenu<CopyData>({ url, onPaste, onDelete, refetchQueri

const handleDeleteClick = async () => {
if (!onDelete) return;
await onDelete({
client,
});
if (refetchQueries) await client.refetchQueries({ include: refetchQueries });
setDeleteDialogOpen(false);
try {
await onDelete({
client,
});
if (refetchQueries) await client.refetchQueries({ include: refetchQueries });
setDeleteDialogOpen(false);
} catch (_) {
throw new Error("Delete failed");
}
};

const handlePasteClick = async () => {
Expand Down Expand Up @@ -164,13 +176,7 @@ export function CrudContextMenu<CopyData>({ url, onPaste, onDelete, refetchQueri
)}
</RowActionsMenu>
</RowActionsMenu>
<DeleteDialog
dialogOpen={deleteDialogOpen}
onDelete={() => {
setDeleteDialogOpen(false);
}}
onCancel={handleDeleteClick}
/>
<DeleteDialog dialogOpen={deleteDialogOpen} onCancel={() => setDeleteDialogOpen(false)} onDelete={handleDeleteClick} />
</>
);
}
2 changes: 1 addition & 1 deletion packages/admin/admin/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export { CancelButton, CancelButtonClassKey, CancelButtonProps } from "./common/
export { ClearInputButton, ClearInputButtonClassKey, ClearInputButtonProps } from "./common/buttons/clearinput/ClearInputButton";
export { CopyToClipboardButton, CopyToClipboardButtonClassKey, CopyToClipboardButtonProps } from "./common/buttons/CopyToClipboardButton";
export { DeleteButton, DeleteButtonClassKey, DeleteButtonProps } from "./common/buttons/delete/DeleteButton";
export { FeedbackButton, FeedbackButtonClassKey, FeedbackButtonProps } from "./common/buttons/feedback/FeedbackButton";
export { FeedbackButton, FeedbackButtonClassKey, FeedbackButtonPropsBase } from "./common/buttons/feedback/FeedbackButton";
export { OkayButton, OkayButtonClassKey, OkayButtonProps } from "./common/buttons/okay/OkayButton";
export { SaveButton, SaveButtonClassKey, SaveButtonProps } from "./common/buttons/save/SaveButton";
export { SplitButton, SplitButtonClassKey, SplitButtonProps } from "./common/buttons/split/SplitButton";
Expand Down
10 changes: 2 additions & 8 deletions storybook/src/docs/components/Toolbar/stories/FeedbackButton.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,16 @@ storiesOf("stories/components/Toolbar/Feedback Button", module)
.addDecorator(toolbarDecorator())
.addDecorator(storyRouterDecorator())
.add("Feedback", () => {
const [loading, setLoading] = React.useState(false);
return (
<Toolbar>
<ToolbarTitleItem>Feedback Button</ToolbarTitleItem>
<ToolbarFillSpace />
<ToolbarActions>
<FeedbackButton
feedbackStateControl="onClickPromise"
color="primary"
variant="contained"
loading={loading}
onClick={() => {
setLoading(true);
setTimeout(() => {
setLoading(false);
}, 1000);
}}
onClick={async () => new Promise((resolve) => setTimeout(resolve, 2000))}
startIcon={<Assets />}
tooltipSuccessMessage="Saving was successful"
tooltipErrorMessage="Error while saving"
Expand Down
Loading