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 #38411

Closed
9 tasks done
Tracked by #38374
mj12albert opened this issue Aug 10, 2023 · 8 comments
Closed
9 tasks done
Tracked by #38374

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

mj12albert opened this issue Aug 10, 2023 · 8 comments
Assignees

Comments

@mj12albert
Copy link
Member

mj12albert commented Aug 10, 2023

Issue for adding FormControl to @mui/material-next as part of the Material You effort (#29345)

Open questions/discussion

Migration Guide checklist

  • Copy component files from material (v5) to material-next, including tests, types, and utils PR
  • Migrate component to Typescript PR
  • Remove deprecated components and componentsProps props, leaving only slots and slotProps PR
  • Drop support for ThemeProvider in favor of CssVarsProvider FormControl doesn't consume any tokens
  • [ ] Refactor component to use Base UI this is more of a new feature
  • Implement Material You design specs - it only contains reset styles and doesn't use any tokens
  • Add component playground to v5 docs will be done at the TextField and Select level
  • Refactor styles to use component CSS Variables - it only contains reset styles and doesn't use any tokens

Tasks

@NextThread

This comment was marked as outdated.

@mj12albert mj12albert self-assigned this Aug 14, 2023
@mj12albert
Copy link
Member Author

I've opened this issue as the remaining features for InputBase require the FormControl component

As a utility component, it isn't straightforward to customize the features of the FormControl. For example Material UI and Joy UI both have custom properties they need in the context for the design to work.

I looked way back into the history of the Base UI FormControl and found this discussion (from 2 yrs ago, in the initial FormControl PR!) that is against merging context, and instead recommends wrapping extra context like this:

<FormControl>
  <FormControlContext.Provider value={thingsSpecificToMd}>
    <FormControlUnstyled>
      <FormControlUnstyledContext.Provider value={genericThings}>
        <!-- actual form controls -->

Should we stick with this approach for material-next/FormControl? @mnajdova @michaldudak @DiegoAndai

Currently the Joy UI form control is pretty much independent of the Base UI one, so it may be the first time trying out this method

@mj12albert

This comment was marked as outdated.

@DiegoAndai
Copy link
Member

Should we stick with this approach for material-next/FormControl?

@mj12albert I'm not sure I get the idea. Will Material's FormControl use Base's FormControl internally and add specific things on top? I imagine something like

import { BaseFormControl } from '@mui/base'

function FormControl(props) {

    const { children } = props;

    // Here we would redirect props to BaseFormControl
    // or Material's context provider accordingly

    return (
        <FormControlContext.Provider value={mdSpecificState}>
            <BaseFormControl {...baseFormControlProps}>
                { children }
            </BaseFormControl>
        </FormControlContext.Provider>
    );
}

Is that correct?

So, the user would still maintain the same API, but internally, we would use the context providers separately.

Adjacent question: does the use of context mean there's no SSR support for FormControl? (as an actual server component)

@mj12albert
Copy link
Member Author

Will Material's FormControl use Base's FormControl internally and add specific things on top?

Yes that's the idea ~ this looks like the only practical way to make a Material UI form control on top of the Base UI one

So, the user would still maintain the same API, but internally, we would use the context providers separately.

There could be some changes but I think it will mostly be the same - what I'm wondering (out loud 😅 ) is whether it will be unnecessarily complicated to use this multiple provider composition instead of just doing it from scratch like Joy UI. At the moment, the Base UI FormControl is relatively simple on the inside (tracking some state in the context).

This also leads me to think about how a user is expected to customize/extend the Base UI FormControl – e.g. adding a isTouched state 🤔

@mj12albert
Copy link
Member Author

mj12albert commented Aug 16, 2023

SSR support for FormControl? (as an actual server component)

@DiegoAndai SSR should work fine, but currently it's using useState to achieve this (e.g. here) so it's the state that's incompatible with RSC

@DiegoAndai
Copy link
Member

Whether it will be unnecessarily complicated to use this multiple provider composition instead of just doing it from scratch like Joy UI

From the Material/Joy user perspective, the key is this comment: "We'd have MD-version of useFormControl that would call both useContext(FormControlUnstyledContext) and useContext(FormControlContext) and return their results". As long as we unify the context values from Base's and Material/Joy's hook, the user will only need Material/Joy's FormControl and useFormControl.

From the Base user perspective (which is us right now 😅), I think using both contexts and joining in the useFormControl is good DX, but may not be intuitive, so I would document it in an "Extending Form Control" section of the Base docs. Base comes with enough common values (value, disabled, required, filled, error) that it's worth using it and not starting from scratch.

@mj12albert
Copy link
Member Author

Closing this as all the migration checklist tasks are done, previously linked "new features/breaking changes" have been moved to #38374

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Recently completed
Status: Done
Development

No branches or pull requests

4 participants