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

Jump to bottom

[Modal][base] Drop component prop #37058

Merged
merged 4 commits into from
Apr 30, 2023
Merged

[Modal][base] Drop component prop #37058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb255ec
[Modal][base] Drop component prop
hbjORbj Apr 27, 2023
34878fd
fix error
hbjORbj Apr 28, 2023
c83ecea
fix type error
hbjORbj Apr 28, 2023
3cfe75f
add comment
hbjORbj Apr 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/pages/base/api/modal.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"children": { "type": { "name": "custom", "description": "element" }, "required": true },
"open": { "type": { "name": "bool" }, "required": true },
"closeAfterTransition": { "type": { "name": "bool" }, "default": "false" },
"component": { "type": { "name": "elementType" } },
"container": { "type": { "name": "union", "description": "HTML element<br>&#124;&nbsp;func" } },
"disableAutoFocus": { "type": { "name": "bool" }, "default": "false" },
"disableEnforceFocus": { "type": { "name": "bool" }, "default": "false" },
Expand Down
1 change: 0 additions & 1 deletion docs/translations/api-docs-base/modal/modal.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"propDescriptions": {
"children": "A single child content element.<br>⚠️ <a href=\"/material-ui/guides/composition/#caveat-with-refs\">Needs to be able to hold a ref</a>.",
"closeAfterTransition": "When set to true the Modal waits until a nested Transition is completed before closing.",
"component": "The component used for the root node. Either a string to use a HTML element or a component.",
"container": "An HTML element or function that returns one. The <code>container</code> will have the portal children appended to it.<br>By default, it uses the body of the top-level document object, so it&#39;s simply <code>document.body</code> most of the time.",
"disableAutoFocus": "If <code>true</code>, the modal will not automatically shift focus to itself when it opens, and replace it to the last focused element when it closes. This also works correctly with any modal children that have the <code>disableAutoFocus</code> prop.<br>Generally this should never be set to <code>true</code> as it makes the modal less accessible to assistive technologies, like screen readers.",
"disableEnforceFocus": "If <code>true</code>, the modal will not prevent focus from leaving the modal while open.<br>Generally this should never be set to <code>true</code> as it makes the modal less accessible to assistive technologies, like screen readers.",
Expand Down
34 changes: 28 additions & 6 deletions packages/mui-base/src/Modal/Modal.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,29 +29,51 @@ const polymorphicComponentTest = () => {
{/* @ts-expect-error */}
<Modal invalidProp={0} />

<Modal component="a" href="#" open>
<Modal<'a'>
slots={{
root: 'a',
}}
href="#"
open
>
<div />
</Modal>

<Modal component={CustomComponent} stringProp="test" numberProp={0} open>
<Modal<typeof CustomComponent>
slots={{
root: CustomComponent,
}}
stringProp="test"
numberProp={0}
open
>
<div />
</Modal>

{/* @ts-expect-error */}
<Modal component={CustomComponent} open>
<Modal<typeof CustomComponent>
slots={{
root: CustomComponent,
}}
open
>
<div />
</Modal>

<Modal
component="button"
<Modal<'button'>
slots={{
root: 'button',
}}
onClick={(e: React.MouseEvent<HTMLButtonElement>) => e.currentTarget.checkValidity()}
open
>
<div />
</Modal>

<Modal<'button'>
component="button"
slots={{
root: 'button',
}}
ref={(elem) => {
expectType<HTMLButtonElement | null, typeof elem>(elem);
}}
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/Modal/Modal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ describe('<Modal />', () => {
},
},
skip: [
'componentProp',
'reactTestRenderer', // portal https://github.com/facebook/react/issues/11565
],
}),
Expand Down
22 changes: 7 additions & 15 deletions packages/mui-base/src/Modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
unstable_createChainedFunction as createChainedFunction,
unstable_useEventCallback as useEventCallback,
} from '@mui/utils';
import { OverridableComponent } from '@mui/types';
import { PolymorphicComponent } from '../utils/PolymorphicComponent';
import { ModalOwnerState, ModalOwnProps, ModalProps, ModalTypeMap } from './Modal.types';
import composeClasses from '../composeClasses';
import Portal from '../Portal';
Expand Down Expand Up @@ -69,7 +69,6 @@ const Modal = React.forwardRef(function Modal<RootComponentType extends React.El
const {
children,
closeAfterTransition = false,
component,
container,
disableAutoFocus = false,
disableEnforceFocus = false,
Expand All @@ -80,7 +79,7 @@ const Modal = React.forwardRef(function Modal<RootComponentType extends React.El
hideBackdrop = false,
keepMounted = false,
// private
manager = defaultManager,
manager: managerProp = defaultManager,
onBackdropClick,
onClose,
onKeyDown,
Expand All @@ -91,7 +90,9 @@ const Modal = React.forwardRef(function Modal<RootComponentType extends React.El
slots = {},
...other
} = props;

// TODO: `modal`` must change its type in this file to match the type of methods
// provided by `ModalManager`
const manager = managerProp as any;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is temporary. I think there is some type error in this file. For example,

we should change

  const modal = React.useRef<{
    modalRef?: typeof modalRef.current;
    mountNode?: typeof mountNodeRef.current;
  }>({});

to

  const modal = React.useRef<{
    mount?: Element;
    modalRef?: Element;
  }>({});

to match the type of methods provided by ModalManager. I'd rather do these changes in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Weird. But yes, I agree, let's solve this in a separate PR. Could you just add a TODO comment above this line explaining what is it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

const [exited, setExited] = React.useState(!open);
const modal = React.useRef<{
modalRef?: typeof modalRef.current;
Expand Down Expand Up @@ -255,7 +256,7 @@ const Modal = React.forwardRef(function Modal<RootComponentType extends React.El
childProps.onExited = createChainedFunction(handleExited, children.props.onExited);
}

const Root = component ?? slots.root ?? 'div';
const Root = slots.root ?? 'div';
const rootProps = useSlotProps({
elementType: Root,
externalSlotProps: slotProps.root,
Expand Down Expand Up @@ -313,7 +314,7 @@ const Modal = React.forwardRef(function Modal<RootComponentType extends React.El
</Root>
</Portal>
);
}) as OverridableComponent<ModalTypeMap>;
}) as PolymorphicComponent<ModalTypeMap>;

Modal.propTypes /* remove-proptypes */ = {
// ----------------------------- Warning --------------------------------
Expand All @@ -329,11 +330,6 @@ Modal.propTypes /* remove-proptypes */ = {
* @default false
*/
closeAfterTransition: PropTypes.bool,
/**
* The component used for the root node.
* Either a string to use a HTML element or a component.
*/
component: PropTypes.elementType,
/**
* An HTML element or function that returns one.
* The `container` will have the portal children appended to it.
Expand Down Expand Up @@ -409,10 +405,6 @@ Modal.propTypes /* remove-proptypes */ = {
* @param {string} reason Can be: `"escapeKeyDown"`, `"backdropClick"`.
*/
onClose: PropTypes.func,
/**
* @ignore
*/
onKeyDown: PropTypes.func,
/**
* If `true`, the component is shown.
*/
Expand Down
6 changes: 3 additions & 3 deletions packages/mui-base/src/Modal/Modal.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import { OverridableComponent, OverridableTypeMap, OverrideProps, Simplify } from '@mui/types';
import { OverridableComponent, OverridableTypeMap, Simplify } from '@mui/types';
import { PortalProps } from '../Portal';
import { SlotComponentProps } from '../utils';
import { PolymorphicProps, SlotComponentProps } from '../utils';

export interface ModalRootSlotPropsOverrides {}
export interface ModalBackdropSlotPropsOverrides {}
Expand Down Expand Up @@ -142,7 +142,7 @@ export type ExtendModal<M extends OverridableTypeMap> = OverridableComponent<Ext

export type ModalProps<
RootComponentType extends React.ElementType = ModalTypeMap['defaultComponent'],
> = OverrideProps<ModalTypeMap<{}, RootComponentType>, RootComponentType>;
> = PolymorphicProps<ModalTypeMap<{}, RootComponentType>, RootComponentType>;

export type ModalOwnerState = Simplify<
ModalOwnProps & {
Expand Down