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.

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 TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control #185

Closed
wants to merge 11 commits into from
Closed
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
"dependencies": {
"encodeurl": "^1.0.2",
"lodash": "^4.17.21",
"prosemirror-utils": "1.2.1-0",
"react-colorful": "^5.6.1",
"tss-react": "^4.8.3",
"type-fest": "^3.12.0"
Expand All @@ -88,6 +89,8 @@
"@tiptap/extension-heading": "^2.0.0-beta.210",
"@tiptap/extension-image": "^2.0.0-beta.210",
"@tiptap/extension-table": "^2.0.0-beta.210",
"@tiptap/extension-table-cell": "^2.0.0-beta.210",
"@tiptap/extension-table-header": "^2.0.4",
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason this is pinned with 2.0.4 minimum instead of 2.0.0-beta.210 like the other dependencies? Would this not work if the end user only has 2.0.0 installed for instance?

"@tiptap/pm": "^2.0.0-beta.210",
"@tiptap/react": "^2.0.0-beta.210",
"react": "^16.8.0 || ^17.0.2 || ^18.0.0",
Expand Down
18 changes: 13 additions & 5 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/ControlledBubbleMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export type ControlledBubbleMenuProps = {
* content.
*/
PaperProps?: Partial<PaperProps>;
onMouseLeave?: (event: React.MouseEvent<HTMLElement>) => void;
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this new prop is no longer needed? Not seeing it referenced elsewhere

};

const controlledBubbleMenuClasses: ControlledBubbleMenuClasses =
Expand Down Expand Up @@ -141,6 +142,7 @@ export default function ControlledBubbleMenu({
],
flipPadding = 8,
PaperProps,
onMouseLeave,
}: ControlledBubbleMenuProps) {
const { classes, cx } = useStyles(undefined, {
props: { classes: overrideClasses },
Expand Down Expand Up @@ -222,6 +224,7 @@ export default function ControlledBubbleMenu({
container={container}
disablePortal={disablePortal}
transition
onMouseLeave={onMouseLeave}
>
{({ TransitionProps }) => (
<Fade
Expand Down
19 changes: 8 additions & 11 deletions src/TableBubbleMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { findParentNodeClosestToPos, posToDOMRect } from "@tiptap/core";
import { findParentNodeOfType } from "prosemirror-utils";
import { useMemo } from "react";
import { makeStyles } from "tss-react/mui";
import type { Except } from "type-fest";
Expand All @@ -9,7 +10,6 @@ import { useRichTextEditorContext } from "./context";
import TableMenuControls, {
type TableMenuControlsProps,
} from "./controls/TableMenuControls";
import { useDebouncedFocus } from "./hooks";
import DebounceRender, {
type DebounceRenderProps,
} from "./utils/DebounceRender";
Expand Down Expand Up @@ -70,15 +70,6 @@ export default function TableBubbleMenu({
const editor = useRichTextEditorContext();
const { classes } = useStyles();

// Because the user interactions with the table menu bar buttons unfocus the
// editor (since it's not part of the editor content), we'll debounce our
// visual focused state so that we keep the bubble menu open during those
// interactions. That way we don't close it upon menu bar button click
// immediately, which can prevent menu button callbacks from working and
// also undesirably will close the bubble menu rather than keeping it open for
// future menu interaction.
const isEditorFocusedDebounced = useDebouncedFocus({ editor });
Comment on lines -73 to -80
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this condition does make it easier to ensure the "nested" menu logic works if editor focus inadvertently gets removed, but this has the downside that even if a user clicks out of the editor and is no longer interacting with it, the bubble menu stays open. Perhaps that's a worthwhile tradeoff. (as mentioned in #185 (comment))


// We want to position the table menu outside the entire table, rather than at the
// current cursor position, so that it's essentially static even as the table changes
// in size and doesn't "block" things within the table while you're trying to edit.
Expand Down Expand Up @@ -143,10 +134,16 @@ export default function TableBubbleMenu({
<TableMenuControls className={classes.controls} labels={labels} />
);

const tableNode = findParentNodeOfType(editor.state.schema.nodes.table)(
editor.state.selection
);

const shouldOpen = Boolean(tableNode);
Comment on lines +137 to +141
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain the rationale behind this vs editor.isActive("table")? Particularly since this brings in a whole new dependency (prosemirror-utils).

I imagine Tiptap may have a util equivalent to findParentNodeOfType, if for some reason we do need that sort of functionality, so ideally we can avoid the new dep.


return (
<ControlledBubbleMenu
editor={editor}
open={isEditorFocusedDebounced && editor.isActive("table")}
open={shouldOpen}
anchorEl={bubbleMenuAnchorEl}
// So the menu doesn't move as columns are added, removed, or resized, we
// prefer "foo-start" rather than the centered "foo" placement. Similarly,
Expand Down
3 changes: 3 additions & 0 deletions src/controls/MenuButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface MenuButtonProps
* included.
*/
tooltipLabel: MenuButtonTooltipProps["label"];
tooltipPlacement?: MenuButtonTooltipProps["placement"];
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a doc comment like we have for the other props

/**
* (Optional) An array of the keyboard shortcut keys that trigger this action
* will be displayed in a tooltip when hovering. If empty, no keyboard
Expand Down Expand Up @@ -70,6 +71,7 @@ const useStyles = makeStyles({ name: { MenuButton } })({
*/
export default function MenuButton({
tooltipLabel,
tooltipPlacement,
tooltipShortcutKeys,
IconComponent,
buttonRef,
Expand All @@ -81,6 +83,7 @@ export default function MenuButton({
<span className={classes.root}>
<MenuButtonTooltip
label={tooltipLabel}
placement={tooltipPlacement}
shortcutKeys={tooltipShortcutKeys}
>
<ToggleButton
Expand Down
11 changes: 8 additions & 3 deletions src/controls/MenuControlsContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,18 @@ export type MenuControlsContainerProps = {
* interval, if `debounced` is true.
*/
DebounceProps?: Except<DebounceRenderProps, "children">;
direction?: "horizontal" | "vertical";
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a doc comment

};

const useStyles = makeStyles({
const useStyles = makeStyles<{
direction: MenuControlsContainerProps["direction"];
}>({
name: { MenuControlsContainer: MenuControlsContainer },
})((theme) => {
})((theme, { direction }) => {
return {
root: {
display: "flex",
flexDirection: direction === "vertical" ? "column" : "row",
rowGap: theme.spacing(0.3),
columnGap: theme.spacing(0.3),
alignItems: "center",
Expand All @@ -45,8 +49,9 @@ export default function MenuControlsContainer({
className,
debounced,
DebounceProps,
direction = "horizontal",
}: MenuControlsContainerProps) {
const { classes, cx } = useStyles();
const { classes, cx } = useStyles({ direction });
const content = <div className={cx(classes.root, className)}>{children}</div>;
return debounced ? (
<DebounceRender {...DebounceProps}>{content}</DebounceRender>
Expand Down
193 changes: 193 additions & 0 deletions src/controls/MenuSelectTableBorderStyle.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
import BorderStyle from "@mui/icons-material/BorderStyle";
import { ClickAwayListener, Fade, Paper, Popper } from "@mui/material";
import * as React from "react";
import { makeStyles } from "tss-react/mui";
import { useRichTextEditorContext } from "../context";
import {
BorderStyleDashed,
BorderStyleDotted,
BorderStyleDouble,
BorderStyleGroove,
BorderStyleInset,
BorderStyleNone,
BorderStyleOutset,
BorderStyleRidge,
BorderStyleSolid,
} from "../icons";
import { Z_INDEXES } from "../styles";
import MenuButton from "./MenuButton";
import type { MenuButtonTooltipProps } from "./MenuButtonTooltip";
import MenuControlsContainer from "./MenuControlsContainer";

export type BorderStyleSelectOption = {
value: string;
IconComponent: React.ElementType<{
className: string;
}>;
label?: string;
shortcutKeys?: MenuButtonTooltipProps["shortcutKeys"];
};

export type MenuSelectTableBorderStyleProps = {
options?: BorderStyleSelectOption[];
label?: string;
className?: string;
};

const useStyles = makeStyles({ name: { MenuSelectTableBorderStyle } })(
(theme) => ({
root: {
zIndex: Z_INDEXES.BUBBLE_MENU,
},

paper: {
backgroundColor: theme.palette.background.default,
},
Comment on lines +43 to +45
Copy link
Owner

@sjdemartini sjdemartini Jan 16, 2024

Choose a reason for hiding this comment

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

This leads to a lint warning currently, since it's not used: https://github.com/sjdemartini/mui-tiptap/actions/runs/7368820861/job/20510696875?pr=185#step:10:12

(I will probably update eslint so that it returns a failing status code even for warnings, since ideally CI would flag this more noisily and prevent warnings from being merged.)

edit: now enforced as of #192

})
);

const DEFAULT_BORDER_STYLE_OPTIONS: BorderStyleSelectOption[] = [
Copy link
Owner

Choose a reason for hiding this comment

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

Is it correct that many of these options don't display differently than "solid" with the default mui-tiptap styles that have borderWidth set to 1px, and they'd require >1px borders to be visually distinct?

{
value: "solid",
label: "Solid",
IconComponent: BorderStyleSolid,
},
{
value: "dashed",
label: "Dashed",
IconComponent: BorderStyleDashed,
},
{
value: "dotted",
label: "Dotted",
IconComponent: BorderStyleDotted,
},
{
value: "double",
label: "Double",
IconComponent: BorderStyleDouble,
},
{
value: "groove",
label: "Groove",
IconComponent: BorderStyleGroove,
},
{
value: "ridge",
label: "Ridge",
IconComponent: BorderStyleRidge,
},
{
value: "inset",
label: "Inset",
IconComponent: BorderStyleInset,
},
{
value: "outset",
label: "Outset",
IconComponent: BorderStyleOutset,
},
{
value: "none",
label: "None",
IconComponent: BorderStyleNone,
},
];

export default function MenuSelectTableBorderStyle({
options = DEFAULT_BORDER_STYLE_OPTIONS,
label,
className,
}: MenuSelectTableBorderStyleProps) {
const editor = useRichTextEditorContext();
const [anchorEl, setAnchorEl] = React.useState<null | HTMLElement>(null);
const open = Boolean(anchorEl);
const { classes, cx } = useStyles();
const handleClick = (event: React.MouseEvent<HTMLElement>) => {
anchorEl ? handleClose() : setAnchorEl(event.currentTarget);
};
const handleClose = () => {
setAnchorEl(null);
};

if (!editor?.isEditable) {
return null;
}

const IconComponent =
options.find(
(option) =>
editor.getAttributes("tableHeader").borderStyle === option.value ||
editor.getAttributes("tableCell").borderStyle === option.value
)?.IconComponent ?? BorderStyle;

return (
<>
<MenuButton
tooltipLabel={label ?? "Border style"}
IconComponent={IconComponent}
onClick={handleClick}
selected={open}
aria-haspopup="true"
aria-expanded={open ? "true" : undefined}
aria-controls={open ? "border-style-menu" : undefined}
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for being mindful of accessibility. I don't see the border-style-menu id used elsewhere, so I think this attribute isn't correct as is. Presumably we'd need to assign an id to the Popper below (assuming I understand aria-controls correct) -- but we should probably let the user provide the id and not have a fixed value. e.g. if a user includes a MenuSelectTableBorderStyle multiple times on the same page, we can't have duplicate IDs. Alternatively we it might be nice to use an auto-generated ID with something like useId, but that's only available in React 18, and I'd rather avoid a custom hook or new dep for it.

/>
<Popper
transition
open={open}
anchorEl={anchorEl}
placement="bottom-start"
className={cx(classes.root, className)}
>
{({ TransitionProps }) => (
<Fade {...TransitionProps} timeout={100}>
<div>
<ClickAwayListener
mouseEvent="onMouseDown"
touchEvent="onTouchStart"
onClickAway={handleClose}
>
<Paper elevation={5}>
<MenuControlsContainer
className={className}
direction="vertical"
Copy link
Owner

Choose a reason for hiding this comment

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

Clever to reuse this component for this purpose, with this new prop!

>
{options.map((option) => (
<MenuButton
key={option.value}
tooltipLabel={option.label ?? option.value}
tooltipPlacement="right"
IconComponent={option.IconComponent}
onClick={() =>
editor
.chain()
.focus()
.setCellAttribute("borderStyle", option.value)
.updateAttributes("tableHeader", {
borderStyle: option.value,
})
Comment on lines +165 to +168
Copy link
Owner

Choose a reason for hiding this comment

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

I take it setCellAttribute doesn't work on table header cells, so the separate updateAttributes call is needed? Would be worth adding a quick comment clarifying this if so

.run()
}
disabled={
!editor
.can()
.setCellAttribute("borderStyle", option.value)
}
selected={
editor.getAttributes("tableHeader").borderStyle ===
option.value ||
editor.getAttributes("tableCell").borderStyle ===
option.value
}
/>
))}
</MenuControlsContainer>
</Paper>
</ClickAwayListener>
</div>
</Fade>
)}
</Popper>
</>
);
}
Loading