react-ui-org / react-ui

React UI is a themeable and performant UI library for React apps.
https://react-ui.io
MIT License
21 stars 7 forks source link

`forwardedRef` is used instead of `ref` #385

Closed bedrich-schindler closed 2 years ago

bedrich-schindler commented 2 years ago

I would like to clarify following:

The thing is that we have HoC src/lib/components/withForwardedRef.jsx that wraps original component with React.forwardRef. This redirects ref to forwardedRef, so every component wrapped by this HoC accepts ref, not forwardedRef.

export default (WrappedComponent) => {
  const withForwardedRef = (props, ref) => (
    <WrappedComponent {...props} forwardedRef={ref} />
  );

  withForwardedRef.displayName = `withForwardedRef(${WrappedComponent.name || WrappedComponent.name})`;

  return React.forwardRef(withForwardedRef);
};
export const Button = ({
  forwardedRef,
  ...restProps
}) => {

export const ButtonWithGlobalProps = withForwardedRef(withGlobalProps(Button, 'Button'));

export default ButtonWithGlobalProps;

First problem is that API shows forwardedRef instead of ref:

image

Other problem is that code examples contain different kind of imports. Sometimes default export (the correct one) is used, sometimes named export is used (the wrong one). It results in fact that almost all examples contain forwardedRef, but ref must be used in implementation, otherwise it won't work.

mbohal commented 2 years ago

Maybe we could change the implementation to remove forwardedRef from props and pass it by context.

We already use this for internal things in:

mbohal commented 2 years ago

Good catch @bedrich-schindler!

bedrich-schindler commented 2 years ago

@mbohal Yeah, it should work. I would try it this way, if @adamkudrna agrees

adamkudrna commented 2 years ago

Please don't forget to update docs, e.g. in Popover:

obrazek
mbohal commented 2 years ago

Aslo we need to make sure that the transferProps position is correct.