patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

feature(Form): Add support for forwardRef

Open gefgu opened this issue 2 years ago • 5 comments

What: Closes #7899

It's my first time working with forwardRef and TypeScript together. In this way, I would be very happy for any kind of feedback :)

gefgu avatar Sep 16 '22 19:09 gefgu

@gefgu It looks like your build error here is as follows:

Error: packages/react-core/src/components/LoginPage/LoginForm.tsx(98,6): error TS2322: Type '{ children: Element[]; accept?: string; acceptCharset?: string; action?: string; allowFullScreen?: boolean; allowTransparency?: boolean; alt?: string; as?: string; async?: boolean; autoComplete?: string; ... 350 more ...; className: string; }' is not assignable to type 'RefAttributes<HTMLFormElement>'.
  Types of property 'ref' are incompatible.
    Type 'LegacyRef<HTMLFormElement>' is not assignable to type 'Ref<HTMLFormElement>'.
      Type 'string' is not assignable to type 'Ref<HTMLFormElement>'.

The LoginForm in the Login component is passing a ref to the form already and is getting a type mismatch. There may be a bit more reconciling you need to do here. Feel free to message the #pf-scrum channel in the patternfly slack workspace if you want some help walking through this.

nicolethoen avatar Sep 20 '22 20:09 nicolethoen

Preview: https://patternfly-react-pr-7995.surge.sh

A11y report: https://patternfly-react-pr-7995-a11y.surge.sh

patternfly-build avatar Sep 21 '22 18:09 patternfly-build

@nicolethoen to address this issue, I've made a change to the props in the LoginForm.tsx. I'm not sure about the side effects of this change, but all the tests have passed now. I would be happy for any kind of feedback and thanks for giving me guidance about this problem.

gefgu avatar Sep 21 '22 18:09 gefgu

@nicolethoen thanks for the guidance. I've implemented the pattern to forward refs similar to the one in Button.tsx and it didn't worked. I tried too with similar patterns I've found in the patternfly codebase, but nothing worked.

I think the type Ref<> type comes with React.forwardRef and I couldn't find a way to make it compatible with LegacyRef and string.

I would be very happy for any feedback about this.

gefgu avatar Sep 22 '22 19:09 gefgu

@nicolethoen Thanks for the feedback. Already implemented the changes :)

gefgu avatar Sep 24 '22 12:09 gefgu