-
Notifications
You must be signed in to change notification settings - Fork 54
feature/expose-mui-component-props #329
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
base: main
Are you sure you want to change the base?
feature/expose-mui-component-props #329
Conversation
@voidlaszlo Thanks for opening this! I like the idea of exposing more props of underlying components, but I think it would be more immediately usable and consistent with other MUI components to directly expose As for exposing things like |
@sjdemartini I can look into that. I'm not sure how easy it is to implement an ExampleUsage
Implementation
In the cases, where we have multiple underlying mui components, we could use |
@voidlaszlo Yeah, as you say, I'm referring to exposing Separately, I think it would be worth also exposing other props of the top-level component and ideally "slots" for inner children components, but I think we can save that for a separate PR to keep things simpler here. The slot props approach I think would warrant a more significant refactor across more mui-tiptap components. WDYT? |
@sjdemartini I'll get to it. Yeah let's do the |
6638a1b
to
ad31460
Compare
I pushed the changes and updated the description. (removed unnecessary and unrelated information for clarity) |
b333f7a
to
995e463
Compare
…tainer, MenuBar, RichTextContent and RichTextField
…nent in FieldContainer, MenuBar, RichTextContent, RichTextField
…nent in ControlledBubbleMenu
… exposed props of mui components
995e463
to
640bc51
Compare
… RichTextContent component in RichTextReadOnly
Updated the branch with rebase. |
Just wanted to mention that I've been quite busy but plan to take a look and get to reviewing this soon (hopefully by next weekend). Thanks again for working on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good! 😄 Just a few minor comments all in the same vein
import { getUtilityClasses } from "./styles"; | ||
|
||
export type ControlledBubbleMenuClasses = ReturnType< | ||
typeof useStyles | ||
>["classes"]; | ||
|
||
export type ControlledBubbleMenuProps = { | ||
export type ControlledBubbleMenuProps = Except<PopperProps, "children"> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, any new props from PopperProps
that would now be included as part of ControlledBubbleMenuProps
are not actually being spread/passed to the popper child below. Seems like we should do that here (like is done with some other components in this PR), to support any additional props beyond sx
, which is already passed explicitly.
Also, we should exclude any props which would be overwritten/ignored, like modifiers
(unless you want to add logic to conditionally use the provided prop instead of the default behavior)
import { Z_INDEXES, getUtilityClasses } from "./styles"; | ||
|
||
export type MenuBarClasses = ReturnType<typeof useStyles>["classes"]; | ||
|
||
export type MenuBarProps = { | ||
export type MenuBarProps = Except<CollapseProps, "children"> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the slightly confusing consequence of including props which are always ignored/overridden, namely in
and unMountOnExit
. Those should be explicitly handled, either by excluding them from this type definition (like you did for children
), or better yet allowing users to override default behavior if they provide them. For instance, setting unmountOnExit=true
when destructuring props, thereby allowing users to override it
import { useRichTextEditorContext } from "./context"; | ||
import { getEditorStyles, getUtilityClasses } from "./styles"; | ||
|
||
export type RichTextContentClasses = ReturnType<typeof useStyles>["classes"]; | ||
|
||
export type RichTextContentProps = { | ||
export type RichTextContentProps = Except<BoxProps, "children"> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should exclude component
in addition to children
, since it's always set directly below
Hey there, is there any update on this? |
Description
Exposed the props of the root MUI components.
FieldContainer
MenuBar
RichTextContent
RichTextField
and changed the
<div>
tag in theFieldContainer
to be a MUIBox
component for the easy modifications.Usage
Related Issues
#302
Checks
pnpm run build
runs successfully