[EuiForm] specify default fullWidth with root component

[spalger]

Summary

Fixes #6214

This PR is an attempt to use context in the <EuiForm> to support changing the default value of fullWidth in any form elements rendered within a given <EuiForm> element.

Checklist

• Props have proper autodocs and playground toggles
• Added documentation
• Checked Code Sandbox works for any docs examples
• Added or updated jest and cypress tests
• Checked for breaking changes and labeled appropriately
• A changelog entry exists and is marked appropriately
• Validated that this has no UI impacts:
• [ ] Checked in both light and dark modes
• [ ] Checked in mobile
• [ ] Checked in Chrome, Safari, Edge, and Firefox
• [ ] Checked for accessibility including keyboard-only and screenreader modes
• [ ] Updated the Figma library counterpart

[spalger]

So... one downside about pulling defaults out of context is that react-view can't automatically determine default values anymore, so I've included them explicitly with @default JSDoc tags... not sure this is going to be great, but I do think that the props on components could use better docs in general so maybe it wouldn't be bad to implement more @default tags across the board...

[cee-chen]

Super awesome workaround/find on this! Totally agreed we could stand to use more @default doc tags for instances where either typescript or react can't infer them automatically 🙌

[spalger]

Welp @constancecchen, this ended up being a lot more complicated than I expected because of the const { ...rest } = props; convention in EUI and the docs site. I'm pretty confident in the state of things now, but I'm not convinced it's worth the drawbacks. That said, I think this is a pattern other parts of EUI could benefit from, maybe?

[spalger]

It'd be great to avoid a type cast here, but defining the type of the context property to FormContextValue requires that we add support for declare properties in classes. I got this working in one version of this PR but it requires a lot of changes to babel configs, so I chose this route instead since I assume the class components will someday be refactored to be function components.

[cee-chen]

++, this would be a good tech debt item for sure.

I am curious though - I know one drawback of context in Class components is that they can only have one this.context. Does this have the potential of breaking if a consumer wraps their own context around a form child, e.g.

<EuiForm fullWidth>
  <SomeRandomContext.Provider>
    <EuiFieldSearch />
  </SomeRandomContext.Provider>
</EuiForm>

TBH i see that as pretty edge case / another tech debt reason to convert these components to function components, so mostly just thinking out loud

[spalger]

My understanding is that classes can only have one context for their static contextType. That doesn't limit the number of contexts that are rendered around the component. React will traverse up the graph to find the context provider that matches the contextType and connect to that, it will skip over the the SomeRandomContext in your example because it doesn't match.

[cee-chen]

Ahh, gotcha! TIL 🎓

[cee-chen]

[just thinking out loud] Does it make sense at all to let EuiFormRow also wrap or set a context value if no EuiForm parent exists? e.g., the following scenario where a consumer is not using EuiForm:

<EuiFormRow fullWidth>
  <EuiFieldSearch /> // inherits fullWidth from EuiFormRow, no need to declare it again
</EuiFormRow>

on second thought that's probably too much of a headache / not worth the effort vs just saying "use EuiForm" haha 🙈

[spalger]

Yeah, I thought about the idea of wrapping the render method of all these components with <FormContext.Provider value={formContext}> so that the fullWidth value at any level would propagate downward. I think it's hard to explain that in docs though, so I decided against it.

[cee-chen]

I'm pretty confident in the state of things now, but I'm not convinced it's worth the drawbacks. That said, I think this is a pattern other parts of EUI could benefit from, maybe?

TBH skimming the code diff, I think the biggest annoyance is the class components. The function components feel like elegant/easy to grok changes to me.

this ended up being a lot more complicated than I expected because of the const { ...rest } = props; convention in EUI and the docs site.

Do you mind diving into this a bit more? I like the way you handled ...rest (by grabbing the form context first and making it a = undefined fallback) but is there another friction point with that that I missed?

The docs/@default changes look fine to me honestly, I think it's a totally appropriate workaround/approach and I also agree that context is a pattern we can/should/do use in a few other places in EUI (e.g. EuiDescriptionList).

@chandlerprall do you mind weighing in on this as well?

[spalger]

this ended up being a lot more complicated than I expected because of the const { ...rest } = props; convention in EUI and the docs site.

Do you mind diving into this a bit more? I like the way you handled ...rest (by grabbing the form context first and making it a = undefined fallback) but is there another friction point with that that I missed?

I tried several different versions of this. The first thing I tried ended up being the one that works the best I think and is the one in the PR. I feel like it would have been better if we could have just done something like const { fullWidth } = useFormContext(props) but I had to include the fullWidth declaration in the destructuring of props in order for ...rest to work, which wasn't ideal, but ...rest is important so I'm not complaining 😅.

Things I tried:

const Component: React.FC = ({ foo, fullWidth, ...rest }) => {
  ({ fullWidth } = useFormContext({ fullWidth })); // weird syntax, not great, if props are not defined in the args then they're not mutable and this wouldn't work
  // ...
}

const Component: React.FC = (props) => {
  const { foo, ...rest } = props;
  const { fullWidth } = useFormContext(props); // leaves `fullWidth` prop in `rest` so it ends up getting passed to things like `<div>`
  // ...
}

const Component: React.FC = (props) => {
  const {
    foo,
    fullWidth,
    ...rest
  } = mergeFormContext(props) // when form context has more than one property that isn't used *everywhere* those extra props get passed down in `rest`
}

[chandlerprall]

@spalger did you explore using an HOC to access an override from context instead? I think that would limit the consuming component changes to just being wrapped with the HOC, avoids distinguishing between function & class components, and should avoid the default/prop/context dances.

Hmm, an HOC would allow some more out-of-the-box configuration too - we recently implemented a similar idea on the description list components and explored it for the flyout, I imagine this is going to be a common thought as we go through the emotion conversion and for new components.

e.g.

export const MyComponent = withPropFromContext(_MyComponent, theContext, 'fullWidth');

[cee-chen]

TBH I feel fairly strongly that I would rather us spend time converting the class components to function components than creating an HOC version of this context.

avoids distinguishing between function & class components

IMO, it creates an even stronger distinction/confusion in that function components will be using context and class components will only be using props.

and should avoid the default/prop/context dances.

Just IMO calling useContext() before destructuring props is hardly a dance. It's a perfectly fine way of writing JS, and (again, IMO) more elegant than prefixing all our components with underscores.

Hmm, an HOC would allow some more out-of-the-box configuration too - we recently implemented a similar idea on the description list components and explored it for the flyout

We definitely did not implement a HOC on EuiDescriptionList, we used a context provider and useContext since all children were function components. Same for the flyout, we removed that context but it was still context-only and had no HOCs. I would have remembered because I'm not a fan of HOCs and I would have pushed against it :) HOCs feel to me like they belong in the old school class component era, and context and hooks were meant to replace them.

[chandlerprall]

I was thinking the HOC would be used for all components, not only classes, as a building block that could provide this pattern. Normally I also lean away from HOCs, but this one seems like a decent place, and if it was built would be re-usable in the description list case - not that we made an HOC for that one :)

The two changed lines for useFormContext + setting the default is very clean and I'm fine with leaving it as-is if we don't want to codify into more of a reusable building block. Either way I'd like to wait for @thompsongl's review on this as well.

[cee-chen]

if it was built would be re-usable in the description list case - not that we made an HOC for that one :)

Ahh gotcha, I misunderstood, apologies.

However, I'm still not seeing how we would reuse it for EuiDescriptionList or why we would want to. These contexts should be different (I'm not seeing why we would want to share/reuse a context for both EuiForm and EuiDescriptionList) and contain different default props - I don't find any syntactical sugar we add around "a HOC that contains context" to be more straightforward than... just using context.

I also want to add, one reason I dislike using HOCs and doing the whole 'underscore the component then export the HOC'd component' thing is that we end up with a million snapshot updates in Kibana where the component name changes to the underscored version. It's a relatively huge pain and in this case I see it as wholly unnecessary when an alternative exists (as opposed to, e.g. WithEuiTheme where an alternative does not exist because it has to be compatible with all EUI components and some components simply cannot be migrated away from a class architecture).

++ with waiting for @thompsongl to weigh in!

[spalger]

I really don't like HOCs and usually find they cause a lot more problems than they solve, but my primary issue with using a HOC for FormContext would be that is makes FormContext a lot less flexible.

Right now it can contain multiple values and it's not necessary for all components to use every one of those values. If we switched to a HOC then every component we wrap would get all of the props "managed" by the FormContext HOC, unless we configure it somehow and then I think it's a lot more complicated than the current implementation.

[thompsongl]

Sorry for the delay! I missed the notifications before he review request.

I think that static contextType is fine for this situation because the most correct approach would be to have only function components and fully embrace hooks. This seems like a temporary and non-obtrusive way to accomplish the context.
With that said, EuiDualRange (as the only exception, I think) will be a pretty involved conversion.

[cee-chen]

@spalger @chandlerprall @thompsongl Should we go ahead and move this PR out of draft / merge it? :)

[chandlerprall]

yes please!

[spalger]

Sweet, I'll get it freshened up today

[cee-chen]

@spalger just want to quickly re-ping on this - this may cause some merge conflicts for us as we move forward with converting form components to Emotion, so we're hoping to get this PR merged in sooner rather than later. Let me know if you'd like me to help out (e.g. merging main, etc) with getting this over the finish line!

[spalger]

Thanks for the reminder, yesterday spiraled a bit. On it now

[spalger]

If you've got opinions about better docs/example please feel free to re-write anything 😅 Just wanted to make sure we had an example of this actually working in several of the components.

[cee-chen]

Looks good to me! I think my only comment would be to make the title have sentence casing, per our copy guidelines

Suggested change

	title: 'Global Full-width',
	title: 'Global full-width',

After looking at it a bit more closely, I'm not totally sure if we should just combine the Full-width and Global full-width examples into 1 single example though - @miukimiu, any thoughts here? We can definitely address that after this PR merges though TBH

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/#/forms/form-layouts#global-full-width

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/

[thompsongl]

Thanks, Spencer!