feat(icon-button): exposes ref prop

[akornmeier]

update: icon button ref was just internal for the tooltip, adds ability to use ref freely

[scurker]

I think we may want to use useImperativeHandle here:

useImperativeHandle(ref, () => buttonRef.current)

That way the ref passed as props below can continue to use the internal ref value, whether it was passed from the parent as a ref or functional ref, or not.

[akornmeier]

What is the difference between the imperativeHandle and what is implemented in the PR? The TS compiler really REALLY does not like: useImperativeHandle(ref, () => buttonRef.current)

[akornmeier]

From what i see useImperativeHandle is more suited to provide a subset of (in this case) <HTMLButton> properties to the forwardRef like "focus". I would think we want to expose the full ref, which is what is implemented.

[akornmeier]

ah, nevermind... i got this to compile, changes inbound and down.

[scurker]

FWIW, I was able to get it working with this as well:

function IconButton({
  icon,
  label,
  tooltipPlacement = 'auto',
  className,
  ...other
}: IconButtonProps, ref: React.Ref<HTMLButtonElement>) {
  const buttonRef = useRef<HTMLButtonElement>(null);
  useImperativeHandle(ref, () => buttonRef.current as HTMLButtonElement)
  return (...)
}

What you have is probably fine too.

[scurker]

I don't care about propTypes here as I find them pretty useless, but others have complained when they were removed. cc @schne324

[akornmeier]

buttonRef was actually not exposed properly w/ prop types and they were being problematic with forwardRef + Typescript, they become essentially extraneous with TS and interfaces

[akornmeier]

also: tooltipPlayemnt

[akornmeier]

I can put them back, however they were not even properly enforcing required props.

[schne324]

Put them back, the rest of the library has them so for now (even though I think they're pointless) let's keep them

[akornmeier]

word. i'll fix them up to enforce the required props n stuff

[akornmeier]

Added the PropTypes object back.

[scurker]

Approving, but I'd like @schne324's to weigh in on removing the propTypes as well.

[schne324]

to weigh in on removing the propTypes as well.

@stephenmathieson iirc you had some concerns with removing them. Thoughts here?

[stephenmathieson]

@stephenmathieson iirc you had some concerns with removing them. Thoughts here?

Only reason I suggest we keep them around is for non-TypeScript users. I agree that they're pointless if you're using TS, but for JS apps, they're pretty handy.

Not a big deal to me either way tho 🤷

[akornmeier]

valid point.