Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(ClipboardCopy): change children type to string #9743

Merged
merged 5 commits into from
Nov 15, 2023
Merged

bug(ClipboardCopy): change children type to string #9743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2851150
bug(ClipboardCopy): change children type to string
gitdallas Oct 13, 2023
fed5a90
fix clipboardcopyexpanded to work with string children
gitdallas Oct 13, 2023
692562f
refactor a bit
gitdallas Oct 19, 2023
cb18398
fix bad rebase
gitdallas Oct 19, 2023
02ea887
Merge branch 'main' into bug/9711-change-clipboard-copy-children-type
gitdallas Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ClipboardCopyToggle } from './ClipboardCopyToggle';
import { ClipboardCopyExpanded } from './ClipboardCopyExpanded';
import { getOUIAProps, OUIAProps } from '../../helpers';

export const clipboardCopyFunc = (event: React.ClipboardEvent<HTMLDivElement>, text?: React.ReactNode) => {
export const clipboardCopyFunc = (event: React.ClipboardEvent<HTMLDivElement>, text?: string) => {
navigator.clipboard.writeText(text.toString());
};

Expand All @@ -27,7 +27,7 @@ export interface ClipboardCopyState {
textWhenExpanded: string;
}

export interface ClipboardCopyProps extends Omit<React.HTMLProps<HTMLDivElement>, 'onChange'>, OUIAProps {
export interface ClipboardCopyProps extends Omit<React.HTMLProps<HTMLDivElement>, 'onChange' | 'children'>, OUIAProps {
/** Additional classes added to the clipboard copy container. */
className?: string;
/** Tooltip message to display when hover the copy button */
Expand Down Expand Up @@ -71,12 +71,12 @@ export interface ClipboardCopyProps extends Omit<React.HTMLProps<HTMLDivElement>
/** Delay in ms before the tooltip appears. */
entryDelay?: number;
/** A function that is triggered on clicking the copy button. */
onCopy?: (event: React.ClipboardEvent<HTMLDivElement>, text?: React.ReactNode) => void;
onCopy?: (event: React.ClipboardEvent<HTMLDivElement>, text?: string) => void;
/** A function that is triggered on changing the text. */
onChange?: (event: React.FormEvent, text?: string) => void;
/** The text which is copied. */
children: React.ReactNode;
/** Additional actions for inline-compact clipboard copy. Should be wrapped with ClipboardCopyAction. */
children: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want to update the onCopy and clipboardCopyFunc params so that text is string | number similar to the onChange prop while we're at it?

Copy link
Contributor Author

@gitdallas gitdallas Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, although maybe just make it always string? The value from a input will always be string anyway

/** Additional actions for inline clipboard copy. Should be wrapped with ClipboardCopyAction. */
additionalActions?: React.ReactNode;
/** Value to overwrite the randomly generated data-ouia-component-id.*/
ouiaId?: number | string;
Expand Down
1 change: 0 additions & 1 deletion packages/react-core/src/components/ClipboardCopy/ClipboardCopyExpanded.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { PickOptional } from '../../helpers/typeUtils';

export interface ClipboardCopyExpandedProps extends Omit<ClipboardCopyProps, 'onChange'> {
className?: string;
children: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep - it matches ClilpboardCopyProps now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the type is changing , we can remove the array logic. However I wonder if this would break people who using the component like mentioned in the issue that resulted in adding array logic. #6863

onChange?: (e: React.FormEvent<HTMLDivElement>, text: string) => void;
isReadOnly?: boolean;
isCode?: boolean;
Expand Down
Loading