Components: require explicit children prop for all components

[ciampo]

Description

As part of the tasks listed in #30503, this PR aims at removing the implicit children prop from ViewOwnProps and from the useContentSystem hook.

The children prop is added to all components that require it explicitly, instead of relying on the implicit definition.

Testing instructions

• The project builds correctly and all tests pass
• Run local storybook npm run storybook:dev and compare the components against what's currently in production
• Play around with components code and manually check the type of the children prop

Screenshots

N/A

Types of changes

Refactor

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• N/A I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• N/A I've included developer documentation if appropriate.
• N/A I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[ciampo]

Sorry for the ping, @ajitbohra and @chrisvanpatten — forgot to mark this as Draft from the start. You can ignore this PR for now

[ciampo]

Hey @sarayourfriend , just leaving a few notes here for the next time we discuss this issue.

Removing the implicit children props

As agreed, I've started by removing the explicit children prop from ViewOwnProps in packages/components/src/ui/context/polymorphic-component.ts (see footnote 1)

But then, when working on each individual components, I realised that TS was still computing the children prop as part of ViewOwnProps. I figured that potentially React.ComponentPropsWithRef includes the children props too, and so I've omit-ted the children prop explicitly (see footnote 2).

Not sure if adding children: never to ViewOwnProps is a better solution here — what do you think?

Finally, I noticed that the children prop was still somehow expected by TS / intellisense on all components that use the Context system — and then I realised that the ConnectedProps typedef also includes the children prop. I tried removing that too (see footnote 3), but that causes a few build errors that I haven't had the time to investigate more in-depth yet.

Adding explicit children props to components

I also started adding explicit children props to all components that needed it.

In case a component's Props are an extension of another component's, I've only added children to the base props (e.g. I've added children to the Flex component's props, but not to VStack and HStack since they extend those props)

Lastly, I've left a few TODO comments when I wasn't sure about the approach to take in specific situations.

[sarayourfriend]

Not sure if adding children: never to ViewOwnProps is a better solution here — what do you think?

I think omitting it is the right way to go. Adding children: never seems like a hack, right?

I'll take a look at the rest soon.

Thanks for starting this PR!

[sarayourfriend]

@ciampo I believe we may just be able to remove the stuff about children in useContextSystem. I'm not sure how this "renderChildren" override is supposed to work and it doesn't appear to be used anywhere in the G2 repository. It's also going to be a huge pain if not nearly impossible to correctly type that stuff, so I think it's best if we just remove that functionality completely. It seems just like overcomplicating something there, especially if it's not actually used.

But then, when working on each individual components, I realised that TS was still computing the children prop as part of ViewOwnProps. I figured that potentially React.ComponentPropsWithRef includes the children props too, and so I've omit-ted the children prop explicitly (see footnote 2).

You're correct, it comes from ComponentProps<'div'> for example, where the div will have children. I think omitting it is the correct option here even though it's also kind of hacky still.

[sarayourfriend]

LGTM so far. Just a couple nits/things to update but I think should be good to go soon 😁

[ciampo]

@sarayourfriend I believe this PR is now ready for a final round of review

[ciampo]

@sarayourfriend could you give a final review round? All tests are passing :)

[sarayourfriend]

LGTM!