Revert "bug(ClipboardCopy): change children type to string"

[tlabaj]

Reverts #9743

[patternfly-build]

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

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

[gitdallas]

@rhamilto - i'm curious what i was missing with the previous change.

#9743 (comment)

in this comment, it looks like you are passing a node in which i assume returns a jsxelement, the clipboard copy does an as String on the children. i'm wondering if you are getting [Object object] in the clipboard (before the breaking change) or if the ExternalLink is using some kind of special toString?

[rhamilto]

@rhamilto - i'm curious what i was missing with the previous change.

#9743 (comment)

in this comment, it looks like you are passing a node in which i assume returns a jsxelement, the clipboard copy does an as String on the children. i'm wondering if you are getting [Object object] in the clipboard (before the breaking change) or if the ExternalLink is using some kind of special toString?

ERROR in packages/shipwright-plugin/src/components/build-details/BuildSpecSection.tsx:118:17
TS2322: Type 'Element' is not assignable to type 'string & ReactNode'.
    116 |               {source.name}:<br />
    117 |               <ClipboardCopy variant={ClipboardCopyVariant.inlineCompact}>
  > 118 |                 <ExternalLink href={source.url} text={source.url} />
        |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    119 |               </ClipboardCopy>
    120 |               <br />
    121 |             </React.Fragment>

was the error I was seeing.

[gitdallas]

@rhamilto - I understand and expect that, but part of the reason for this change is that the ClipboardCopy tool converts the children to string before using them. For that reason I'm curious how it worked before this change - I'd expect that if someone clicks to copy, it would actually put [object Object] into the clipboard. Would you be able to test?

[rhamilto]

@rhamilto - I understand and expect that, but part of the reason for this change is that the ClipboardCopy tool converts the children to string before using them. For that reason I'm curious how it worked before this change - I'd expect that if someone clicks to copy, it would actually put [object Object] into the clipboard. Would you be able to test?

No, I cannot test it as the code won't build.

[gitdallas]

I'm asking to test with the previous version of PF, as that was doing an as String on the children passed in. I'm curious what the outcome is (what gets put into the clipboard).

For example, this is using an older version of PF, and when I pass in an element as the child... clicking to copy to clipboard gives me [object Object] https://codesandbox.io/p/sandbox/loving-rain-59d759

[gitdallas]

approved incase it's urgent, but i'm still curious as to how this may have worked properly

[rhamilto]

@gitdallas, let me investigate. The instances are in static plugins that the admin console team doesn't own, so I've got to figure out where I can test.

[rhamilto]

@gitdallas, I manually duplicated https://github.com/openshift/console/blob/master/frontend/packages/shipwright-plugin/src/components/build-details/BuildSpecSection.tsx#L78-L80 and confirmed the clipboard contents are indeed [object Object]. So it appears those existing instances are existing bugs. Not sure exactly when they became bugs, however. Clearly something changed along the lines. Will need to file bugs.

[gitdallas]

@tlabaj - i'm not sure we want to revert this. i believe that anywhere that this actually would break someone's build it would already be a bug, and perhaps a good thing that we are identifying bugs.

other than just leaving like that, idk if it would be worth looking back in time to see if this was valid at some point and recreating whatever functionality that was.

[tlabaj]

Thank you @gitdallas and @rhamilto fir verifying this, I will close the PR.

[wise-king-sullyman]

Reopening as it's been reported that this change is breaking Ansible

[gitdallas]

This (the PR being reverted) addressed the original bug as written: #9711

but the problem is, we DO accept ReactNode and handle it properly IF the consumer also provides an onCopy override.

rather than force the child type to be string, consider that we should make sure that the expansion variant uses the onCopy (including if overridden) to generate the preview.

documentation should also be updated to make these points clear

[gitdallas]

Good as long as we have a new story to address:

• expansion preview (or maybe expansion variant SHOULD NOT allow react node child?)
• improved documentation to let users know that if they do indeed pass something other than string as the child, they will NEED to override onCopy