feat(ClipboardCopy): added event to onChange prop #8747
Conversation
| @@ -119,7 +119,7 @@ export class ClipboardCopy extends React.Component<ClipboardCopyProps, Clipboard | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
| componentDidUpdate = (prevProps: ClipboardCopyProps, prevState: ClipboardCopyState) => { | |||
| if (prevProps.children !== this.props.children) { | |||
| this.updateText(this.props.children as string | number); | |||
| this.setState({ text: this.props.children as string | number }); | |||
There was a problem hiding this comment.
I changed this to just set the state (rather than call updateText) as there is no event to pass to updateText and the onChange.
I think this should be fine, since the only way this conditional is met is that the change in value is coming via children passed to the component, in which case consumers should already be aware that the value is being changed, so the onChange coming from ClipboardCopy shouldn't be necessary.
If I'm thinking about that incorrectly though please let me know!
|
Preview: https://patternfly-react-pr-8747.surge.sh A11y report: https://patternfly-react-pr-8747-a11y.surge.sh |
There was a problem hiding this comment.
This looks good. The only thing that it looks like is that whenever TextInput has its update to its onChange callback, updateText may need to be updated again to switch the params, as will ClipboardCopyExpanded. Would be found by whomever works on that issue, but we could open the issue now to mention it.
|
Ah good callout on ClipboardCopyExpanded, that should be within the scope of this PR IMO. TextInput already has a checkbox under the epic so I don't think a new issue is needed for that now? |
What: Closes #8698
Additional issues: