eui
eui copied to clipboard
elastic
[EuiForm] specify default fullWidth with root component
Summary
Fixes https://github.com/elastic/eui/issues/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
- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [x] Props have proper autodocs and playground toggles
- [x] Added documentation
- [ ] Checked Code Sandbox works for any docs examples
- [x] Added or updated jest and cypress tests
- [x] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Updated the Figma library counterpart
- [x] A changelog entry exists and is marked appropriately
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/
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?
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/
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?
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`
}
@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');
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.
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.
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!
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.
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.
@spalger @chandlerprall @thompsongl Should we go ahead and move this PR out of draft / merge it? :)
@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!
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/#/forms/form-layouts#global-full-width
Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/