[popover2] feat: new prop targetProps

[bvandercar-vt]

Fixes #0000

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

Allow targetProps to be passed to popover2 that get passed to the span wrapper of the popover target.

Mutually exclusive with renderTarget , since renderTarget doesn't get wrapped.

With existing functionality, this could still be accomplished by specifying a custom renderTarget that wraps their child in a span, ie before:

 <Popover2 {...popoverProps}>
    <Button  {...buttonProps} />
  </Popover2>

after:

<Popover2
    {...popoverProps}
    renderTarget={(props) => (
      <span {...props} {...targetProps}>
            <Button  {...buttonProps} />
      </span>
    )}
  />

But with this proposed change, one could do:

 <Popover2 {...popoverProps} targetProps={targetProps}>
    <Button  {...buttonProps} />
  </Popover2>

So, for us, this isn't an "essential" change, since we can accomplish the same with renderTarget-- but I wanted to propose it to the Blueprint team.

[bvandercar-vt]

since we apply the fill class to the child target when renderTarget isn't defined, I figured we should apply this class to the custom target as well

[adidahiya]

Thanks for the PR @bvandercar-vt. This enhancement makes sense to me, especially after doing a bunch of Popover2 migrations of our internal code. When I initially designed Popover2, I figured most target customizations should happen via renderTarget, and that would be sufficient, but I now realize that it's a lot better from a developer experience perspective if we allow minor modifications to target element attributes through a new prop like this.

One use case I imagine this will be useful for is something like:

<Popover2 targetProps={{ style: { /* absolute positioning */ } }}>

On naming... to avoid potential confusion/conflation with "wrapper" terminology from Popover "v1", and because the existing prop which customizes the .bp4-popover-target element is named targetTagName (rather than targetWrapperTagName), I think this should be called targetProps.

[adidahiya]

Suggested change

	targetWrapperProps?: React.HTMLProps<HTMLSpanElement> | React.HTMLProps<HTMLDivElement>;
	targetProps?: TProps;

[bvandercar-vt]

OK done, please review.

In the case of a renderTarget, targetProps props will also be appliedto the renderTarget, so I made the warning basically say that they will be applied, but that it doesn't really make sense to do it that way vs. just applying them in your renderTarget.

[bvandercar-vt]

@adidahiya I'm not sure why CI is failing, is TProps not the best type for targetProps here maybe?

[adidahiya]

TProps is the correct type to use here, but adding this constraint exposed some type issues with the way I had designed Popover2. It required adding a stricter typedef to validateProps, which further exposed some complex issues with the renderTarget API and the inferred value of TProps. I think I've solved those issues in the commits I just pushed to this branch.

[adidahiya]

I've updated your PR to no longer inject props.targetProps into props.renderTarget -- it was causing type issues and it's not necessary to support. I've also updated the error message to reflect this change.

[adidahiya]

@bvandercar-vt this is nearly good to go, I just need to test it out locally to see if it breaks anything downstream... stay tuned, I'll try to get this in for the release I'm cutting today

[adidahiya]

This change seems to work well downstream for the renderTarget API... it allows us to clean up some code, too, for example:

class MyComponent extends React.PureComponent {
    public render() {
        return (
-           <Popover2<React.HTMLProps<HTMLDivElement>>
+           <Popover2
                renderTarget={this.renderTarget}
                content="content"
            />
        );
    }

    private renderTarget = ({
        isOpen,
        ref,
        ...targetProps
-    }: Popover2TargetProps & React.HTMLProps<HTMLDivElement>) => {
+    }: Popover2TargetProps & Popover2ClickTargetHandlers) => {
        return (
            <div
                ref={ref}
                {...targetProps}
            />
        );
    };
}