Add TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control #185

bryanjtc · 2023-12-29T21:22:15Z

Adds two new extensions and one control to modify the table border style.

Relates to #184

The MenuSelectTableBorderStyle component has been added to the controls folder. This component is necessary to implement a specific table border style in the application.

bryanjtc · 2023-12-30T16:02:32Z

The MenuSelect and Extension work as expected.

Recording.2023-12-30.105935.mp4

I have a small problem with the table bubble menu, It keeps closing when I open the menu select control. Any idea on how to fix it? @sjdemartini

Recording.2023-12-30.110149.mp4

- Add BorderStyleDashed icon sourced from https://www.svgrepo.com/svg/361694/border-dashed - Add BorderStyleDotted icon sourced from https://www.svgrepo.com/svg/361691/border-dotted - Add BorderStyleSolid icon sourced from https://www.svgrepo.com/svg/361697/border-solid These icons are added to provide different border style options for UI elements.

sjdemartini · 2023-12-30T20:26:05Z

@bryanjtc Nice work getting what you have together so far!

That's a tricky issue. As you've probably seen, the logic for the controlling the table bubble menu open is whether (1) the editor is in focus and (2) Tiptap's state shows a "table" element is active, which is based on the current caret/selection location:

mui-tiptap/src/TableBubbleMenu.tsx

Line 149 in 8767227

open={isEditorFocusedDebounced && editor.isActive("table")}

In this case, the editor is losing focus due to the Select's Menu (which uses a Popover). You can see this by removing the isEditorFocusedDebounced && part of the condition. I'm not sure what the best option would be in this case. In general it seems we shouldn't keep bubble menus open if the user has clicked away from the editor, so I think that part of the logic is generally useful, though that's obviously causing this undesirable side effect here. Maybe the open={...} logic could be extended somehow to account for a case that there's a menu open? I'm not sure what the right way to do that would be. Do you have any ideas?

sjdemartini · 2023-12-30T20:29:53Z

Side note: if the table bubble menu is appearing near the bottom of the page currently, and the border style select is opened, it overlaps in an awkward way that can make it tricky to interact with (though maybe partially because of the MenuSelect's tooltip logic):

I'm not sure what the right UI/UX should be since this is leading to "nested" menus. 🤔

- Refactored the following components: ControlledBubbleMenu, MenuControlsContainer, MenuSelectTableBorderStyle, TableMenuControls, and EditorMenuControls. - Made changes to improve code readability and maintainability. - No functional changes were made, only code restructuring and organization.

bryanjtc · 2023-12-30T23:53:29Z

I refactored the code and created another bubble menu inside the table bubble menu. It works better.

Recording.2023-12-30.185222.mp4

This commit adds a new extension called TableHeaderImproved.ts to the src/extensions directory. The extension enhances the functionality of the table header in the demo application. The package.json file and src/demo/useExtensions.ts file were also modified as part of this feature implementation.

…ectTableBorderStyle.tsx.

This commit adds new icons for different border styles such as dashed, dotted, double, groove, inset, none, outset, ridge, and solid. These icons will be used in the application to represent different border styles in a visual and intuitive way.

…ectTableBorderStyle.tsx.

bryanjtc · 2024-01-04T23:34:17Z

@sjdemartini Can you review the pr? I now that it doesn't have comments, i'm waiting for review to add the comments. Just in case there is something else that needs to be moved or changed.

sjdemartini · 2024-01-04T23:41:35Z

@bryanjtc Cool, wasn't sure if you'd settled in a place where you thought it was ready for review again. Things have been super busy on my end unfortunately, but I'll try to review some time soon (hopefully next week). Sorry for the delay.

sjdemartini

Nice work getting this to its current state despite some of the nuances with Tiptap and mui-tiptap! I think for us to be able to merge it, the main criteria would be

This doesn't break anything for users who are using the regular Table, TableCell, and/or TableHeader extensions
This has a good UI/UX that feels consistent with other mui-tiptap features
The code is well structured and offers consistent APIs with other parts of mui-tipap
Documented in the README and in code

I think (2) is nearly there, with the caveat around the default table styles seemingly making some of the border styles have no visual effect (so perhaps we'd want to change the default options for table border style). And (4) is obviously straightforward, and as you said can be done once the rest is figured out.

(1) needs a bit of tweaking per TableMenuControls including the new option even if the user doesn't have the new extensions. (3) has the caveat around the new "icons" not actually being icons. These are likely resolvable, but curious about your thoughts given where things stand. (If it seems tricky to achieve consistency and all, another option could conceivably be your own package for these new extensions and table-extending functionality, which could play nicely with mui-tiptap 😄)

Thanks again @bryanjtc!

sjdemartini · 2024-01-15T23:48:39Z

package.json

@@ -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",


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?

sjdemartini · 2024-01-15T23:50:46Z

src/TableBubbleMenu.tsx

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


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.

sjdemartini · 2024-01-15T23:55:14Z

src/TableBubbleMenu.tsx

-  // 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 });


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))

sjdemartini · 2024-01-15T23:55:43Z

src/controls/MenuButton.tsx

@@ -18,6 +18,7 @@ export interface MenuButtonProps
   * included.
   */
  tooltipLabel: MenuButtonTooltipProps["label"];
+  tooltipPlacement?: MenuButtonTooltipProps["placement"];


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

sjdemartini · 2024-01-15T23:56:16Z

src/controls/MenuControlsContainer.tsx

@@ -23,14 +23,18 @@ export type MenuControlsContainerProps = {
   * interval, if `debounced` is true.
   */
  DebounceProps?: Except<DebounceRenderProps, "children">;
+  direction?: "horizontal" | "vertical";


Please add a doc comment

sjdemartini · 2024-01-16T00:20:06Z

src/icons/BorderStyleDashed.tsx

+
+function BorderStyleDashed() {
+  return (
+    <Box


Creative use of Box to achieve more useful icons for these border styles. One caveat unfortunately is that these border "icons" won't have the same props as the other icons which are stand MUI SvgIcons. For instance, won't support the color or fontSize props. That might be okay, but it's a quirk that could be confusing to users.

sjdemartini · 2024-01-16T00:25:47Z

src/controls/MenuSelectTableBorderStyle.tsx

+  })
+);
+
+const DEFAULT_BORDER_STYLE_OPTIONS: BorderStyleSelectOption[] = [


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?

sjdemartini · 2024-01-16T00:26:28Z

src/ControlledBubbleMenu.tsx

@@ -87,6 +87,7 @@ export type ControlledBubbleMenuProps = {
   * content.
   */
  PaperProps?: Partial<PaperProps>;
+  onMouseLeave?: (event: React.MouseEvent<HTMLElement>) => void;


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

sjdemartini · 2024-01-16T00:36:23Z

src/extensions/TableHeaderImproved.ts

@@ -0,0 +1,26 @@
+import { TableHeader } from "@tiptap/extension-table-header";
+
+const TableHeaderImproved = TableHeader.extend({


We'd want to document the new extensions and whatnot in the README if we're going to merge this

sjdemartini · 2024-01-16T00:41:00Z

src/icons/BorderStyleDashed.tsx

+        width: MENU_BUTTON_FONT_SIZE_DEFAULT,
+        height: MENU_BUTTON_FONT_SIZE_DEFAULT,


It looks like these new border style icons appear a bit larger than the rest of the standard MUI icons. Should these be smaller?

bryanjtc · 2024-01-17T17:29:01Z

Nice that you took the time to review everything. I will try to push the fixes/changes today and reply/resolve each conversatioon.

As mentioned in #185 (comment)

Add new icons and extensions

fecda46

bryanjtc changed the title ~~Add TableCell Improxed extension and MenuSelectCellStyle control~~ Add TableCell Improved extension and MenuSelectCellStyle control Dec 29, 2023

feat: Add MenuSelectTableBorderStyle component

9065a4a

The MenuSelectTableBorderStyle component has been added to the controls folder. This component is necessary to implement a specific table border style in the application.

bryanjtc changed the title ~~Add TableCell Improved extension and MenuSelectCellStyle control~~ Add TableCell Improved extension and MenuSelectTableBorderStyle control Dec 30, 2023

Update aria-label and tooltipTitle for MenuSelectTableBorderStyle

f493ffd

bryanjtc added 2 commits December 30, 2023 19:02

Refactor MenuSelectTableBorderStyle component in src/controls/MenuSel…

ee36e70

…ectTableBorderStyle.tsx.

bryanjtc changed the title ~~Add TableCell Improved extension and MenuSelectTableBorderStyle control~~ Add TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control Dec 31, 2023

bryanjtc marked this pull request as ready for review December 31, 2023 01:35

bryanjtc added 3 commits December 31, 2023 00:25

Add prosemirror-utils dependency and update TableBubbleMenu

e267efe

Add tooltipPlacement prop to MenuButton component

e295a52

Refactor MenuSelectTableBorderStyle component in src/controls/MenuSel…

4ba6af4

…ectTableBorderStyle.tsx.

sjdemartini reviewed Jan 16, 2024

View reviewed changes

sjdemartini added a commit that referenced this pull request Jan 25, 2024

Fail CI if there are any eslint warnings

7ccc07e

As mentioned in #185 (comment)

sjdemartini mentioned this pull request Jan 25, 2024

Fail CI if there are any eslint warnings #192

Merged

sjdemartini added a commit that referenced this pull request Jan 25, 2024

Fail CI if there are any eslint warnings

4caacb6

As mentioned in #185 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control #185

Add TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control #185

bryanjtc commented Dec 29, 2023 •

edited

Loading

bryanjtc commented Dec 30, 2023 •

edited

Loading

sjdemartini commented Dec 30, 2023

sjdemartini commented Dec 30, 2023

bryanjtc commented Dec 30, 2023

bryanjtc commented Jan 4, 2024

sjdemartini commented Jan 4, 2024

sjdemartini left a comment

sjdemartini Jan 15, 2024

sjdemartini Jan 15, 2024

sjdemartini Jan 15, 2024

sjdemartini Jan 15, 2024

sjdemartini Jan 15, 2024

sjdemartini Jan 16, 2024

sjdemartini Jan 16, 2024

sjdemartini Jan 16, 2024

sjdemartini Jan 16, 2024

sjdemartini Jan 16, 2024

bryanjtc commented Jan 17, 2024

		@@ -0,0 +1,26 @@
		import { TableHeader } from "@tiptap/extension-table-header";

		const TableHeaderImproved = TableHeader.extend({

		width: MENU_BUTTON_FONT_SIZE_DEFAULT,
		height: MENU_BUTTON_FONT_SIZE_DEFAULT,

Add TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control #185

Are you sure you want to change the base?

Add TableCellImproved, TableHeaderImproved extensions and MenuSelectTableBorderStyle control #185

Conversation

bryanjtc commented Dec 29, 2023 • edited Loading

bryanjtc commented Dec 30, 2023 • edited Loading

sjdemartini commented Dec 30, 2023

sjdemartini commented Dec 30, 2023

bryanjtc commented Dec 30, 2023

bryanjtc commented Jan 4, 2024

sjdemartini commented Jan 4, 2024

sjdemartini left a 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

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

bryanjtc commented Jan 17, 2024

bryanjtc commented Dec 29, 2023 •

edited

Loading

bryanjtc commented Dec 30, 2023 •

edited

Loading