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]: Can fullWidth be added to the FormControl context? #19336

Closed
1 task done
EsoterikStare opened this issue Jan 21, 2020 · 13 comments
Closed
1 task done

[FormControl]: Can fullWidth be added to the FormControl context? #19336

EsoterikStare opened this issue Jan 21, 2020 · 13 comments
Labels
component: text field This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@EsoterikStare
Copy link
Contributor

EsoterikStare commented Jan 21, 2020

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

I would like to be able to consume the fullWidth FormControl state prop from the FormControl context in order to have better support for more custom fullWidth behaviors, especially for non-MUI children.

Examples 🌈

https://github.com/mui-org/material-ui/blob/2082679d80db7848cf47187a128a22491bf0b492/packages/material-ui/src/FormControl/FormControl.js#L157

const childContext = {
    adornedStart,
    setAdornedStart,
    color,
    disabled,
    error,
    filled,
    focused,
+   fullWidth,
    hiddenLabel,
    margin: (size === 'small' ? 'dense' : undefined) || margin,
    onBlur: () => {
      setFocused(false);
    },
    onEmpty,
    onFilled,
    onFocus: () => {
      setFocused(true);
    },
    registerEffect,
    required,
    variant,
  };

Seems simple and innocuous to just also add the available fullWidth prop value to the childContext object, which, in turn, gets set as the value of the FormControlContext.Provider.

Then, I could just do something like:

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

like I can for most other useful FormControl state props, using formControlState, or any other custom reducer I might need.

Without this, I'm basically forced to have a specific Form theme override for each problematic child.

Motivation 🔦

Right now, it's simply assumed that the FormControl children behave a certain way and that's not always true for non-MUI children. I've run into a fairly specific problem with the way react-select works that would be greatly simplified if I could just reference the value of the parent FormControl's fullWidth prop and condition my wrapper component styles based off of that to avoid forking the (strange) internal workings of react-select.

Bottom line, is there any harm in just adding fullWidth to the FormControl context? It seems to be a strange outlier, and there doesn't seem to be an obvious reason to exclude it.

I can do the one-line change if this seems like something that could be done.

@eps1lon
Copy link
Member

eps1lon commented Jan 21, 2020

Thank you for opening this issue and explaining the motivation.

Could you list all the form related components that implement the fullWidth prop?

If react-select causes too much headache you might want to try out our unstable Autocomplete component. It's intended to replace react-select integration. It would help if you can test this instead and see if this helps.

@EsoterikStare
Copy link
Contributor Author

EsoterikStare commented Jan 21, 2020

We're very aware of (and excited about!) Autocomplete, but we have a more immediate need to support our existing, react-select-based component until we can justify using Autocomplete in production. =( Sorry, I should've mentioned that.

I'll try and get an audit together for all the fullWidth-compatible components today. Thanks for the response!

@EsoterikStare
Copy link
Contributor Author

EsoterikStare commented Jan 21, 2020

These are all the form-related components which implement a fullWidth prop:

  • FilledInput
  • FormControl
  • Input
  • InputBase
  • OutlinedInput
  • TextField

To me, this seems like a perfectly innocuous addition since there shouldn't be any components looking for or acting on its presence in the context, and this appears to be the case, on cursory inspection of the code.

Is there a particular reason this was not originally included in the context? It seems odd that it was excluded while things like filled were included? ... So, maybe? I could imagine that the desired functionality was achieved without passing that down, so it was simply deferred until there was a request or a use case.

Anyways, I'm speculating. Let me know what you think.

@eps1lon
Copy link
Member

eps1lon commented Jan 21, 2020

It seems odd that it was excluded while things like filled were? ... So, maybe?

We put things in context if different components in a FormControl need the same "prop". This doesn't seem to be the case for fullWidth as this is only implement by input-like components. You should only have one input per FormControl.

e.g. variant is in context because input-like as well as the InputLabel require that prop.

Could you share your code? Maybe I'm missing some places where you have a single FormControl but multiple Material-UI components that use fullWidth.

@EsoterikStare
Copy link
Contributor Author

Sorry, I can't share my code, but I can provide some pseudo-code.

I think I understand what you're saying. The problem I think this solves is that Inputs can be set to fullWidth by only passing fullWidth onto the FormControl, like this:

const foo = () => {
  return (
    <FormControl fullWidth>
      <InputLabel>State</InputLabel>
      <Input />
      <FormHelperText>Please choose your state of residence</FormHelperText>
    </FormControl>
  )
}

But, when using a custom form component like react-select, I have to also pass fullWidth to the child like this:

const foo = () => {
  return (
    <FormControl fullWidth>
      <InputLabel>State</InputLabel>
      <CustomSelect fullWidth />
      <FormHelperText>Please choose your state of residence</FormHelperText>
    </FormControl>
  )
}

It would be great if a custom form component could be made to behave just like a MUI form component by consuming the FormControl context in a wrapper and passing the relevant values down. Currently, the only exception to that being true is the absence of the fullWidth value from the FormControl context.

Just to be totally clear, I want to be able to do this:

const foo = () => {
  return (
    <FormControl fullWidth>
      <InputLabel>State</InputLabel>
      <CustomSelect />
      <FormHelperText>Please choose your state of residence</FormHelperText>
    </FormControl>
  )
}

@eps1lon
Copy link
Member

eps1lon commented Jan 21, 2020

So your current code looks like

const foo = () => {
  return (
    <FormControl>
      <InputLabel>State</InputLabel>
      <CustomSelect fullWidth />
      <FormHelperText>Please choose your state of residence</FormHelperText>
    </FormControl>
  )
}

?
What would putting it in context achieve here?

@EsoterikStare
Copy link
Contributor Author

EsoterikStare commented Jan 22, 2020

No, that code example is not quite right. Sorry, this is hard to describe well.

Right now, due to the styles and structure of the underlying library, the only way for a consumer of our custom component to implement fullWidth is to pass it to both the FormControl and the custom form component, like this:

const foo = () => {
  return (
    <FormControl fullWidth>
      <InputLabel>State</InputLabel>
      <CustomSelect fullWidth />
      <FormHelperText>Please choose your state of residence</FormHelperText>
    </FormControl>
  )
}

FormControl needs that width: '100%' style so its child will have the ability to grow as well, and the custom form component has some custom styles that need to be driven by fullWidth.

By simply adding fullWidth to the context, it allows me to consume that in my custom component and drive the fullWidth behavior internally without requiring consumers to remember to also pass fullWidth to the custom form component, which is inconsistent with the behavior/implementation of other MUI form components.

The goal is to be able to provide a custom form component that can be implemented in the same way any other MUI form component can be (i.e, Input). Like this:

const foo = () => {
  return (
    <FormControl fullWidth>
      <InputLabel>State</InputLabel>
      <CustomSelect />
      <FormHelperText>Please choose your state of residence</FormHelperText>
    </FormControl>
  )
}

My hope is that it's a very simple and low-risk enhancement that would enable people like me to build stuff that behaves exactly as you'd expect inside of a FormControl.

Again, the only obstacle I can see to that being a reality is the absence of fullWidth from the context. Seems like every other prop you'd want is present already.

@EsoterikStare
Copy link
Contributor Author

EsoterikStare commented Jan 23, 2020

Basically, even though it’s not needed by other form components besides the input, the fact that both the FormControl and the input need to do something to support fullWidth makes me believe it should be in the context. That way the input can just respect the context. Thoughts?

@oliviertassinari
Copy link
Member

Related: this resonates with an issue we have with the Autocomplete component. The textfied needs to be fullwidth in order for the width of the root element (the textbox and listbox container) to behave correctly. I couldn't find a good solution.

@oliviertassinari
Copy link
Member

What if we turn the input full-width prop on by default if there is a form control?

@eps1lon
Copy link
Member

eps1lon commented Jan 23, 2020

Basically, even though it’s not needed by other form components besides the input, the fact that both the FormControl and the input need to do something to support fullWidth makes me believe it should be in the context.

Gotcha, I missed that one. In that case we should move it into context.

@EsoterikStare
Copy link
Contributor Author

@oliviertassinari, Turning on full-width when in a FormControl (by evaluating whether the context is defined or not) is part of what we have done internally to support this in other components that are a little less quirky than react-select, so I can say from experience that that seems to work well.

I can add fullWidth to the childContext object in FormControl's source and get the MR submitted today, most likely. Thanks for all the discourse!

@EsoterikStare EsoterikStare changed the title [FormControl]: Is there a reason FormControl context shouldn't contain fullWidth? [FormControl]: Can fullWidth be added to the FormControl context? Jan 23, 2020
@EsoterikStare
Copy link
Contributor Author

This was merged in, so I'm closing this issue. Cheers!

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants