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.

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

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

Merged
merged 18 commits into from
Oct 3, 2023

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Sep 18, 2023

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
Copy link

mui-bot commented Sep 18, 2023

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

@mj12albert mj12albert linked an issue Sep 18, 2023 that may be closed by this pull request
9 tasks
@mj12albert mj12albert force-pushed the material-next/form-control branch from 098f825 to e00cfa4 Compare September 18, 2023 08:57
@mj12albert mj12albert marked this pull request as ready for review September 18, 2023 08:58
@zannager zannager requested a review from DiegoAndai September 18, 2023 12:24
@mj12albert mj12albert removed the request for review from DiegoAndai September 18, 2023 12:52
@mj12albert mj12albert marked this pull request as draft September 18, 2023 12:53
@github-actions 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 mj12albert force-pushed the material-next/form-control branch from e00cfa4 to 8e940b8 Compare September 21, 2023 05:33
@github-actions 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 mj12albert force-pushed the material-next/form-control branch from 9210958 to 548da4a Compare September 21, 2023 09:06
@github-actions 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 mj12albert force-pushed the material-next/form-control branch from 7586fae to 2d2905d Compare September 24, 2023 08:44
@github-actions 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 mj12albert marked this pull request as ready for review September 24, 2023 09:05
@mj12albert mj12albert requested review from DiegoAndai, michaldudak and mnajdova and removed request for michaldudak September 24, 2023 09:05
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Looks great overall 🎉
Just some small questions/comments

packages/mui-base/src/FormControl/index.ts Outdated Show resolved Hide resolved
packages/mui-material-next/src/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
packages/mui-material-next/src/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
@@ -1,4 +1,6 @@
// TODO v6: decide whether to update/refactor this, keep as-is, or drop it
Copy link
Member

Choose a reason for hiding this comment

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

What's the idea behind dropping/refactoring it?

Copy link
Member Author

@mj12albert mj12albert Sep 27, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 mj12albert force-pushed the material-next/form-control branch from 86b8c52 to c91f973 Compare September 27, 2023 08:11
@mj12albert
Copy link
Member Author

mj12albert commented Sep 27, 2023

@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
Copy link
Member

DiegoAndai commented Sep 27, 2023

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.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
Great work 😊

@mj12albert
Copy link
Member Author

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" />

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.

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.

@@ -375,7 +366,7 @@ const InputBase = React.forwardRef(function InputBase<
};

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

Choose a reason for hiding this comment

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

Why are we no longer checking for the disabled state?

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 It never should have been checking for !fcs.disabled here - it was part of #36892 which was reverted!

@mj12albert
Copy link
Member Author

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 mj12albert merged commit 587398b into mui:master Oct 3, 2023
6 checks passed
@mj12albert mj12albert deleted the material-next/form-control branch October 3, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants