Add new Custom DialogTitle to @comet/admin #2609

fichtnerma · 2024-10-07T20:32:12Z

Description

Create option to add an onClose callback with a close Button by creating a custom Dialog to replace MUI Dialogs

Example

I have verified if my change requires an example

Screenshots/screencasts

Changeset

I have verified if my change requires a changeset

Related tasks and documents

https://vivid-planet.atlassian.net/browse/COM-1045

jamesricky · 2024-10-08T06:02:18Z

packages/admin/admin/src/common/DialogTitle.tsx

+const Title = createComponentSlot(MUIDialogTitle)<DialogTitleClassKey>({
+    componentName: "DialogTitle",
+    slotName: "dialogTitle",
+})(css`
+    display: flex;
+    justify-content: space-between;
+    align-items: center;
+`);


Maybe this slot should be called "Root" to be consistent with other components

packages/admin/admin/src/common/DialogTitle.tsx

jamesricky · 2024-10-08T06:13:37Z

.changeset/curvy-bulldogs-boil.md

+Add new custom `DialogTitle` to @comet/admin
+
+-   optional `onClose` prop adds Close Button to the Component
+
+Example:
+
+-   `<DialogTitle onClose={() => {...}} />`


Mentioning "@comet/admin" is not necessary as this changelog is explicitly for "@comet/admin"

johnnyomair · 2024-10-08T06:42:05Z

It is not possible to understand why we need this component without reading the JIRA task. Please make sure that a PR can be understood without having to look into the task.

johnnyomair · 2024-10-08T06:42:44Z

@jamesricky in the MUI docs they achieve this button using an IconButton and absolute positioning: https://mui.com/material-ui/react-dialog/#customization. Wouldn't this be the preferred way?

jamesricky · 2024-10-08T10:53:20Z

@jamesricky in the MUI docs they achieve this button using an IconButton and absolute positioning: https://mui.com/material-ui/react-dialog/#customization. Wouldn't this be the preferred way?

Yes, probably a good idea 👍🏼
The title will need some padding-right though to prevent potential overlapping.

johnnyomair

Wouldn't it be better if it were a custom dialog, and not a dialog title? IMO the onClose prop would make more sense on the dialog.

johnnyomair · 2024-10-29T07:13:24Z

Wouldn't it be better if it were a custom dialog, and not a dialog title? IMO the onClose prop would make more sense on the dialog.

@jamesricky @fichtnerma what do you think about this suggestion?

jamesricky · 2024-10-29T13:41:25Z

Wouldn't it be better if it were a custom dialog, and not a dialog title? IMO the onClose prop would make more sense on the dialog.

@jamesricky @fichtnerma what do you think about this suggestion?

That probably makes sense 👍🏼
So like this I assume, so the <DialogTitle /> is rendered internally, when the title and/or onClose prop is set?

<Dialog
    open={showDialog}
    title="This is a dialog"
    onClose={() => {
        setShowDialog(false);
    }}
>
    {dialogContent}
</Dialog>

This could also be helpful for aligning the Buttons in DialogActions correctly which currently if often done incorrectly as it's not very intuitive. E.g., by adding leftActions and rightActions props.
(Probably as a follow-up task)

fichtnerma · 2024-10-29T21:17:28Z

Wouldn't it be better if it were a custom dialog, and not a dialog title? IMO the onClose prop would make more sense on the dialog.

@jamesricky @fichtnerma what do you think about this suggestion?

Yeah makes more sense to me logically

jamesricky · 2024-10-30T07:16:32Z

packages/admin/admin/src/common/Dialog.tsx

+    Dialog as MUIDialog,
+    DialogTitle as MUIDialogTitle,
+    IconButton as MUIIconButton,


Please use Mui instead of MUI to stay consistent with other import renames.

jamesricky · 2024-10-30T07:16:56Z

packages/admin/admin/src/common/Dialog.tsx

+const Root = createComponentSlot(MUIDialog)<DialogClassKey>({
+    componentName: "Dialog",
+    slotName: "root",
+})(css``);


Suggested change

})(css``);

})();

jamesricky · 2024-10-30T07:22:38Z

storybook/src/admin/Dialog.tsx

+const textContent =
+    "Dialog content. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.";
+
+storiesOf("@comet/admin/Dialog", module)


This should probably be combined with the existing Dialog story: https://storybook.comet-dxp.com/?path=/story/comet-admin-mui--dialog

Maybe extend the existing one and add knobs for title and onClose, similar to Dialog size:

comet/storybook/src/admin/mui/Dialog.tsx

Line 29 in f8ae084

const selectedDialogSize = select("Dialog size", dialogSizeOptions, "sm");

jamesricky · 2024-10-30T07:27:54Z

packages/admin/admin/src/index.ts

@@ -22,6 +22,7 @@ export { SplitButtonContext, SplitButtonContextOptions } from "./common/buttons/
 export { useSplitButtonContext } from "./common/buttons/split/useSplitButtonContext";
 export { ClearInputAdornment, ClearInputAdornmentProps } from "./common/ClearInputAdornment";
 export { CometLogo } from "./common/CometLogo";
+export { Dialog, DialogClassKey, DialogProps } from "./common/Dialog";


We should probably add an import restriction to prevent using MUIs Dialog.
This is how it's done for Alert for example:

comet/packages/eslint-config/react.js

Lines 42 to 46 in f8ae084

{

name: "@mui/material",

importNames: ["Alert"],

message: "Please use Alert from @comet/admin instead",

},

jamesricky · 2024-10-30T07:28:39Z

packages/admin/admin/src/common/Dialog.tsx

+const IconButton = createComponentSlot(MUIIconButton)<DialogClassKey>({
+    componentName: "Dialog",
+    slotName: "iconButton",


IconButton is too generic imo, I'd prefer something like CloseButton

jamesricky · 2024-10-30T07:30:29Z

packages/admin/admin/src/common/Dialog.tsx

+})(css`
+    color: inherit;
+    width: 100%;
+    padding-right: 40px;


This padding should only be added when there is a close-button, otherwise the spacings left and right of the text are inconsistent.

jamesricky · 2024-10-30T07:32:22Z

packages/admin/admin/src/common/Dialog.tsx

+    color: inherit;
+    width: 100%;


These should be the default when using a div.

Suggested change

color: inherit;

width: 100%;

jamesricky · 2024-10-30T07:33:26Z

packages/admin/admin/src/common/Dialog.tsx

+    right: 20px;
+`);
+
+const TitleWrapper = createComponentSlot("div")<DialogClassKey>({


Technically a div is not allowed inside h2.

Suggested change

const TitleWrapper = createComponentSlot("div")<DialogClassKey>({

const TitleWrapper = createComponentSlot("span")<DialogClassKey>({

johnnyomair · 2024-10-30T10:26:27Z

So like this I assume, so the <DialogTitle /> is rendered internally, when the title and/or onClose prop is set?

I wouldn't do this. I'd render the button outside of the dialog title using absolute positioning, as MUI does in the example.

This could also be helpful for aligning the Buttons in DialogActions correctly which currently if often done incorrectly as it's not very intuitive. E.g., by adding leftActions and rightActions props.

IMO we shouldn't add to many custom props/components for things which can be implemented easily without custom styling. But you could argue that the positioning is often done incorrectly 🤔

jamesricky · 2024-11-12T07:32:22Z

packages/admin/admin/src/common/Dialog.tsx

+    right: 20px;
+`);
+
+const TitleWrapper = createComponentSlot("div")<DialogClassKey>({


Can't the styles of TitleWrapper just be part of DialogTitle directly? 🤔

Add new Custom DialogTitle to @comet/admin

eebd54e

auto-assign bot requested a review from johnnyomair October 7, 2024 20:32

auto-assign bot assigned fichtnerma Oct 7, 2024

fichtnerma mentioned this pull request Oct 7, 2024

Custom DialogTitle eslint rule #2610

Draft

3 tasks

jamesricky self-requested a review October 8, 2024 05:34

jamesricky reviewed Oct 8, 2024

View reviewed changes

johnnyomair reviewed Oct 10, 2024

View reviewed changes

Change styling of DialogTitle

4b8f950

Refactor to create custom Dialog instead of DialogTitle

a47226c

jamesricky reviewed Oct 30, 2024

View reviewed changes

Update Dialog Story

b070d7d

jamesricky reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Custom DialogTitle to @comet/admin #2609

Add new Custom DialogTitle to @comet/admin #2609

fichtnerma commented Oct 7, 2024 •

edited

Loading

jamesricky Oct 8, 2024

jamesricky Oct 8, 2024

johnnyomair commented Oct 8, 2024

johnnyomair commented Oct 8, 2024

jamesricky commented Oct 8, 2024

johnnyomair left a comment

johnnyomair commented Oct 29, 2024

jamesricky commented Oct 29, 2024

fichtnerma commented Oct 29, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

jamesricky Oct 30, 2024

johnnyomair commented Oct 30, 2024

jamesricky Nov 12, 2024

	{
	name: "@mui/material",
	importNames: ["Alert"],
	message: "Please use Alert from @comet/admin instead",
	},

	const TitleWrapper = createComponentSlot("div")<DialogClassKey>({
	const TitleWrapper = createComponentSlot("span")<DialogClassKey>({

Add new Custom DialogTitle to @comet/admin #2609

Are you sure you want to change the base?

Add new Custom DialogTitle to @comet/admin #2609

Conversation

fichtnerma commented Oct 7, 2024 • edited Loading

Description

Example

Screenshots/screencasts

Changeset

Related tasks and documents

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyomair commented Oct 8, 2024

johnnyomair commented Oct 8, 2024

jamesricky commented Oct 8, 2024

johnnyomair left a comment

Choose a reason for hiding this comment

johnnyomair commented Oct 29, 2024

jamesricky commented Oct 29, 2024

fichtnerma commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyomair commented Oct 30, 2024

Choose a reason for hiding this comment

fichtnerma commented Oct 7, 2024 •

edited

Loading