Skip to content

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

Merged
mj12albert merged 38 commits intomui:masterfrom
mj12albert:material-next/filled-input
Oct 23, 2023
Merged

[FilledInput][material-next] Add FilledInput component#39307
mj12albert merged 38 commits intomui:masterfrom
mj12albert:material-next/filled-input

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 scope: text field Changes related to the text field. 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
@mj12albert mj12albert force-pushed the material-next/filled-input branch from 30a908b to 8e764ab Compare October 6, 2023 06:02
@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
@mj12albert mj12albert force-pushed the material-next/filled-input branch from fb9afbb to 08e77aa Compare October 9, 2023 10:13
@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
@mj12albert mj12albert force-pushed the material-next/filled-input branch from 51252cf to 1493a04 Compare October 10, 2023 07:05
@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/src/FilledInput/FilledInput.tsx Outdated

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

@DiegoAndai DiegoAndai Oct 12, 2023

Choose a reason for hiding this comment

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

What was the error? 🤔

@mj12albert mj12albert requested a review from DiegoAndai October 13, 2023 08:55
@mj12albert mj12albert force-pushed the material-next/filled-input branch from c362850 to 6c233b2 Compare October 13, 2023 11:17
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

@michaldudak michaldudak Oct 13, 2023

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

@mj12albert mj12albert Oct 17, 2023

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 Outdated

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

@michaldudak michaldudak Oct 13, 2023

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

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

@mnajdova mnajdova Oct 20, 2023

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

@mj12albert mj12albert Oct 20, 2023

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

@mnajdova mnajdova Oct 20, 2023

Choose a reason for hiding this comment

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

Perfect, thanks!

@mj12albert mj12albert force-pushed the material-next/filled-input branch from 7557cb4 to 1797c40 Compare October 20, 2023 10:32
@mj12albert
Copy link
Member Author

mj12albert commented Oct 20, 2023

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

@mj12albert mj12albert requested a review from mnajdova October 20, 2023 11:25
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
@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

design: material you scope: text field Changes related to the text field.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mj12albert @mui-bot @mnajdova @michaldudak @DiegoAndai

Comments