[FormControl][material-next] Add FormControl component by mj12albert · Pull Request #39032 · mui/material-ui

mj12albert · 2023-09-18T07:52:58Z

Part of #38411, #29345

This PR refactors FormControl for v6

Summary:

It now internally uses useSlotProps
Almost all of the previously skipped InputBase tests are restored!
The refactor to use Base UI's FormControl component will be done separately, there are still some skipped tests at this point (dependent on some other components) so it would be better to restore those first
I have followed (at least) the PR section of the contributing guide.

mui-bot · 2023-09-18T07:56:11Z

Netlify deploy preview

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

@mui/material-next: parsed: +0.23% , gzip: +0.19%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against d2955f8

DiegoAndai

Looks great overall 🎉
Just some small questions/comments

packages/mui-base/src/FormControl/index.ts

packages/mui-material-next/src/FormControl/FormControl.tsx

packages/mui-material-next/src/FormControl/FormControl.types.ts

packages/mui-material-next/src/FormControl/FormControlContext.ts

DiegoAndai · 2023-09-26T18:17:15Z

packages/mui-material-next/src/FormControl/formControlState.js

@@ -1,4 +1,6 @@
+// TODO v6: decide whether to update/refactor this, keep as-is, or drop it


What's the idea behind dropping/refactoring it?

This is always used together with useFormControl like:

const muiFormControl = useFormControl() const fcs = formControlState({ props, muiFormControl, states: ['color', 'disabled'], });

I was thinking it could be better to do this in a single hook, e.g.

const [ formControlState, // the result of `formControlState()` formControlContext, // the result of `useFormControl()` ] = useFormControlState({ props, states: ['color', 'disabled'], })

This would be internal, and wouldn't affect the original (public) useFormControl unless we wanted to (e.g. rename it to useFormControlContext or sth)

What do you think?

I see. Adding a higher-level abstraction (useFormControlState) makes sense to me if we're using useFormControl and formControlState together all the time 👍🏼

We could create an issue and add it to the v6 milestone so we don't forget and can discuss the specifics whenever we have time.

mj12albert · 2023-09-27T14:10:21Z

@DiegoAndai I fixed all the comments, and also fixed a mistake from before – the inputComponent prop shouldn't have been dropped from InputBase, it's the same as the component prop which is passed to emotion's as prop... only here it's called inputComponent 😓

(I'm tempted to rename it component to make TS a little easier, but let's keep it as-is for now in case it makes integrating with other components harder)

DiegoAndai · 2023-09-27T14:52:13Z

I'm tempted to rename it component to make TS a little easier, but let's keep it as-is for now in case it makes integrating with other components harder

Yeah, let's do that, either now or in the future. If not now, then let's create an issue and add it to the v6 milestone. The convention should be to use component, like ButtonBase and most other components do.

DiegoAndai

LGTM 🚀
Great work 😊

mj12albert · 2023-09-28T11:49:04Z

The convention should be to use component, like ButtonBase and most other components do.

I've added an item in the TextField issue to follow up ~

Right now I would guess that it's named like that to indicate that it's going to the input slot instead of the root 🤔

// works
<FilledInput id="filled" defaultValue="Hello" inputComponent="textarea" />

// doesn't work – TS complains FilledInput/InputBase doesn't accept `component`
<FilledInput id="filled" defaultValue="Hello" component="span" />

mnajdova

Just one comment for clarification. It would be great if we have a playground page (maybe in the experiments folder) to check the component and test manually some of the functionality.

mnajdova · 2023-09-29T11:27:20Z

packages/mui-material-next/src/InputBase/InputBase.tsx


  const handleClick = (event: React.PointerEvent) => {
-    if (onClick /* && !fcs.disabled */) {
+    if (onClick) {


Why are we no longer checking for the disabled state?

@mnajdova It never should have been checking for !fcs.disabled here - it was part of #36892 which was reverted!

mj12albert · 2023-10-03T06:33:22Z

It would be great if we have a playground page (maybe in the experiments folder) to check the component and test manually some of the functionality

@mnajdova I will include this with the next part which is FilledInput ~ it will have some visible styles which will make a playground easier to use!

mj12albert added component: FormControl The React component. design: material you labels Sep 18, 2023

mj12albert mentioned this pull request Sep 18, 2023

[FormControl][material-next] Add FormControl component #38411

Closed

9 tasks

mj12albert linked an issue Sep 18, 2023 that may be closed by this pull request

[FormControl][material-next] Add FormControl component #38411

Closed

9 tasks

mj12albert removed a link to an issue Sep 18, 2023

[FormControl][material-next] Add FormControl component #38411

Closed

9 tasks

mj12albert mentioned this pull request Sep 18, 2023

[InputBase][material-next] Add InputBase component #38372

Closed

11 tasks

mj12albert force-pushed the material-next/form-control branch from 098f825 to e00cfa4 Compare September 18, 2023 08:57

mj12albert marked this pull request as ready for review September 18, 2023 08:58

zannager requested a review from DiegoAndai September 18, 2023 12:24

mj12albert removed the request for review from DiegoAndai September 18, 2023 12:52

mj12albert marked this pull request as draft September 18, 2023 12:53

mj12albert mentioned this pull request Sep 18, 2023

[FormControl][material-next] Copy v5 FormControl #39039

Merged

1 task

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 21, 2023

mj12albert force-pushed the material-next/form-control branch from e00cfa4 to 8e940b8 Compare September 21, 2023 05:33

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 21, 2023

mj12albert force-pushed the material-next/form-control branch from 9210958 to 548da4a Compare September 21, 2023 09:06

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 22, 2023

mj12albert added 12 commits September 24, 2023 16:40

Convert to typescript

22da155

Implement slots

ae76f29

Integrate with InputBase

cd3e5f2

Update InputBase tests

9bec34c

Update FormControl tests

f7e7e52

Update proptypes

fed7cfb

Add back registering input tests

92ac592

Update comments

23a9797

Add back component prop

97ae8db

Update proptypes

1f4d057

Update comments

4a36291

Remove redundant test

50c04aa

Fix component prop and tests

2d2905d

mj12albert force-pushed the material-next/form-control branch from 7586fae to 2d2905d Compare September 24, 2023 08:44

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 24, 2023

mj12albert marked this pull request as ready for review September 24, 2023 09:05

mj12albert requested review from DiegoAndai, michaldudak and mnajdova and removed request for michaldudak September 24, 2023 09:05

DiegoAndai reviewed Sep 26, 2023

View reviewed changes

mj12albert added 2 commits September 27, 2023 15:38

Add back inputComponent prop

6ce9f78

Fix review comments

c91f973

mj12albert force-pushed the material-next/form-control branch from 86b8c52 to c91f973 Compare September 27, 2023 08:11

mj12albert added 2 commits September 27, 2023 21:23

Update migration.md

cfa76e2

Update proptypes

76db999

mj12albert requested a review from DiegoAndai September 27, 2023 14:12

Update comments

d2955f8

DiegoAndai approved these changes Sep 27, 2023

View reviewed changes

mj12albert mentioned this pull request Sep 26, 2023

[TextField][material-next] Add v7 TextField component #38374

Open

16 tasks

mnajdova reviewed Sep 29, 2023

View reviewed changes

mj12albert mentioned this pull request Oct 3, 2023

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

Closed

9 tasks

mj12albert merged commit 587398b into mui:master Oct 3, 2023

mj12albert deleted the material-next/form-control branch October 3, 2023 07:11

mj12albert mentioned this pull request Oct 3, 2023

[FormControl][material-next] New internal form control state hook #39286

Open

mj12albert mentioned this pull request Oct 13, 2023

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

Merged

1 task

mj12albert mentioned this pull request Oct 27, 2023

[base-ui] Fix mergeSlotProps className join order #39616

Merged

1 task

		@@ -1,4 +1,6 @@
		// TODO v6: decide whether to update/refactor this, keep as-is, or drop it

Uh oh!

Conversation

mj12albert commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Bundle size report

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DiegoAndai Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

mj12albert Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoAndai Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

mj12albert commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiegoAndai commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Uh oh!

mj12albert commented Sep 28, 2023

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

mnajdova Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

mj12albert Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

mj12albert commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

mj12albert commented Sep 18, 2023 •

edited

Loading

mui-bot commented Sep 18, 2023 •

edited

Loading

mj12albert Sep 27, 2023 •

edited

Loading

mj12albert commented Sep 27, 2023 •

edited

Loading

DiegoAndai commented Sep 27, 2023 •

edited

Loading