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

[FilledInput][material-next] Add FilledInput component #39307

Merged
merged 38 commits into from
Oct 23, 2023
Merged

[FilledInput][material-next] Add FilledInput component #39307

merged 38 commits into from
Oct 23, 2023

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Oct 4, 2023 •
edited by mnajdova
Loading

Part of #39052

  • Most of the PR focuses on converting everything (including FormControl.tests and InputBase.tests) to TS, and adopting useSlotProps
  • Design-wise, existing v5 tokens are replaced but Material You styles/tokens are not fully implemented yet and will be done in a separate PR, since the changes could be quite extensive (as a reference material-web uses 80+ tokens!). It could also be easier to implement the design together with the related Label/HelperText components.
  • There are now only 2 remaining sets of skipped tests across FilledInput, FormControl, and InputBase: one depends on Select, the other depends on TextField
  • Experiments page added under /experiments/md3/inputs

Screenshot 2023-10-09 at 10 42 12 PM

Preview: https://deploy-preview-39307--material-ui.netlify.app/experiments/md3/inputs/

@mj12albert mj12albert added component: text field This is the name of the generic UI component, not the React module! design: material you labels Oct 4, 2023
@mui-bot
Copy link

mui-bot commented Oct 4, 2023 •
edited
Loading

Netlify deploy preview

https://deploy-preview-39307--material-ui.netlify.app/

@mui/material-next: parsed: +0.31% , gzip: +0.94%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5d132de

@mj12albert mj12albert force-pushed the material-next/filled-input branch 2 times, most recently from c202d8a to 05d05c6 Compare October 5, 2023 02:21
@mj12albert mj12albert mentioned this pull request Oct 5, 2023
Closed
9 tasks
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 7, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 9, 2023
@mj12albert mj12albert force-pushed the material-next/filled-input branch 7 times, most recently from 3347073 to 51252cf Compare October 9, 2023 15:59
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 9, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 10, 2023
DiegoAndai
DiegoAndai reviewed Oct 12, 2023
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! left some questions and suggestions 😊

packages/mui-material-next/migration.md Outdated Show resolved Hide resolved
packages/mui-material-next/src/InputBase/InputBase.test.tsx Outdated Show resolved Hide resolved
packages/mui-material-next/src/InputBase/InputBase.test.tsx Show resolved Hide resolved
packages/mui-material-next/src/InputBase/InputBase.test.tsx Outdated Show resolved Hide resolved
packages/mui-material-next/src/InputBase/InputBase.tsx Show resolved Hide resolved
packages/mui-material-next/src/FormControl/FormControl.test.tsx Outdated Show resolved Hide resolved
packages/mui-material-next/src/FilledInput/FilledInput.tsx Outdated

const RootSlot = slots.root ?? components.Root ?? FilledInputRoot;
const InputSlot = slots.input ?? components.Input ?? FilledInputInput;
if (multiline) {
Copy link
Member

Choose a reason for hiding this comment

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

What was the error? 🤔

packages/mui-material-next/src/InputBase/InputBase.types.ts Outdated Show resolved Hide resolved
packages/mui-material-next/src/FilledInput/FilledInput.tsx Show resolved Hide resolved
packages/mui-material-next/src/FilledInput/FilledInput.test.tsx Outdated Show resolved Hide resolved
michaldudak
michaldudak reviewed Oct 13, 2023
View reviewed changes
packages/mui-material-next/src/FilledInput/FilledInput.tsx
type,
},
externalForwardedProps: other,
ownerState: ownerState as FilledInputOwnerState & InputBaseOwnerState,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't FilledInputOwnerState include the InputBaseOwnerState fields? This shouldn't be necessary then.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some common properties, but even more differences as these OwnerStates include their OwnProps which is where the differences are:

material-ui/packages/mui-material-next/src/FilledInput/FilledInput.types.ts

Lines 67 to 73 in f46a4c2

export type FilledInputOwnerState = Simplify<
FilledInputOwnProps & {
disableUnderline?: boolean;
fullWidth: boolean;
inputComponent: React.ElementType;
multiline: boolean;
type?: React.InputHTMLAttributes<HTMLInputElement>['type'];

(btw the TS error here was also quite intimidating 😂 luckily I found this trick from Joy https://github.com/mui/material-ui/blob/master/packages/mui-joy/src/Menu/Menu.tsx#L156)

packages/mui-material-next/src/FilledInput/FilledInput.tsx Show resolved Hide resolved
packages/mui-material-next/src/FilledInput/FilledInput.tsx Outdated

const RootSlot = slots.root ?? components.Root ?? FilledInputRoot;
const InputSlot = slots.input ?? components.Input ?? FilledInputInput;
if (multiline) {
Copy link
Member

Choose a reason for hiding this comment

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

<InputBase
  slots={{ root: Root, input: Input }}
  slotProps={{
    input: inputProps,
  }}
  type={type}
  multiline={multiline}
  {...rootProps}
/>

The error you see comes from the fact you're providing the type prop, which makes TS choose the overload with multiline=false. And since the multiline variable can be true | false, TS complains.

It would be nice if we could do

type={multiline ? undefined : type}
multiline={multiline}

but TS isn't that smart, apparently :)

@mj12albert mj12albert mentioned this pull request Oct 17, 2023
Closed
9 tasks
mnajdova
mnajdova reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I am ok with making the types flat, however, can we do some runtime warning if the users does not provide multiline, but they provide some of the props that makes sense only if the component rendered is textarea?

packages/mui-material-next/src/InputBase/InputBase.test.tsx Outdated
injectedProps = props;
const { ownerState, ...other } = props;
return <input ref={ref} {...other} />;
});

render(<InputBase inputComponent={MyInputBase} />);

expect(typeof injectedProps.onBlur).to.equal('function');
expect(typeof injectedProps.onFocus).to.equal('function');
if (injectedProps) {
Copy link
Member

Choose a reason for hiding this comment

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

When would this be undefined? Are we accidentally skipping these expect statements?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mnajdova Fixed ~ it's not needed, I accidentally left it there while trying to fix some TS issue!

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks!

@mj12albert
Copy link
Member Author

can we do some runtime warning if the users does not provide multiline, but they provide some of the props that makes sense only if the component rendered is textarea?

Good idea ~ added in 2f6bc51

mnajdova
mnajdova approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Great work! 🚀

@mj12albert mj12albert merged commit b5c39e1 into mui:master Oct 23, 2023
22 checks passed
@mj12albert mj12albert deleted the material-next/filled-input branch October 23, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova mnajdova approved these changes

@DiegoAndai DiegoAndai Awaiting requested review from DiegoAndai

@michaldudak michaldudak Awaiting requested review from michaldudak

Assignees
No one assigned
Labels
component: text field This is the name of the generic UI component, not the React module! design: material you
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mj12albert @mui-bot @mnajdova @michaldudak @DiegoAndai