copy button that copies text to clipboard#1112
Conversation
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
There are some issues with the tooltip rendering:
| content={this.state.tooltipText} | ||
| onMouseOut={this.resetTooltipText} | ||
| > | ||
| <EuiButton |
There was a problem hiding this comment.
I think we may want to pull this out into a more generic service so that they can pass any type of child node (any button or link) that gets wrapped in this functionality. As it stands, you can't use EuiButtonEmpty or EuiButtonIcon.
I would also like to look into creating a "disappearing" tooltip to use here. Where there is no tooltip on hover, but then when you click the trigger element, the tooltip only stays up for a short period of time. I'll look into this in another branch.
There was a problem hiding this comment.
Maybe use a toast instead, which will auto dismiss itself and is good for this action (as well as for accessibility because it'll announce itself). The button will likely have the action in it so the tooltip on hover is likely unnecessary. Agree it should be more generic and accept any child element, not just a regular button.
There was a problem hiding this comment.
My concern with a toast, is that it's not very tied to the button itself and can be easily overlooked. For small actions like this, maybe we should consider a new type of tooltip that reads out like a toast, but whose proximity is much closer to the triggering element?
There was a problem hiding this comment.
There are some issues with the tooltip rendering
is a known bug, #996 ; coincidentally this exact use case
|
@cchaos @chandlerprall I have moved the component under utilities and now anything can be used as the child. |
|
Jenkins, test this please. |
|
|
||
| <EuiSpacer size="m" /> | ||
|
|
||
| <EuiCopy textToCopy={this.state.copyText}> |
There was a problem hiding this comment.
What do you think about making this a render prop instead of adding a span with its own onClick?
<EuiCopy textToCopy={this.state.copyText}>
{(copyFn) => (
<EuiButton onClick={copyFn}>Click to copy input text</EuiButton>
)}
</EuiCopy>
That keeps the utility component from affecting DOM and makes it more re-useable in other places.
|
Should |
I think leaving it as a string makes sense; the parent is still in control, and the copy operation uses the current value on |
| import { EuiToolTip } from '../tool_tip'; | ||
|
|
||
| const UNCOPIED_MSG = 'Copy'; | ||
| const COPIED_MSG = 'Copied'; |
There was a problem hiding this comment.
Can you make these messages customizable via props?
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Just a couple nit things, but thank you so much for adding this in. I'm excited to be able to start using it in our docs!
| }], | ||
| text: ( | ||
| <p> | ||
| The <EuiCode>EuiCopy</EuiCode> component is a simplified utility for copying text to clipboard. |
There was a problem hiding this comment.
Can you give more description on how it's used?
| } | ||
|
|
||
| EuiCopy.propTypes = { | ||
| textToCopy: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Can you add comments to each prop type to populate the props list table in the docs?
Also, you may want to consider add TS def's ;)
| }; | ||
|
|
||
| EuiCopy.defaultProps = { | ||
| beforeCopyMsg: 'Copy', |
There was a problem hiding this comment.
I think it's most likely that by default we won't need a before message (That'll also give us a quick fix for the tooltip positioning issue that probably won't be fixed relatively soon). Can you remove the before message default and in the component make the tooltip dependent on having either of these messages?
Also, since we've been being pretty verbose with our props, I think we can change these to "beforeMessage" and "afterMessage" ("Copy" is redundant of the component name and "Msg" can be remembered easily).
There was a problem hiding this comment.
The problem with not having a default before message is that then the toolip is displayed with no text. I am not sure how this should work without a before and after message
There was a problem hiding this comment.
Yeah you would need to conditionally wrap the child in a tooltip when one or both messages exist.
| } = this.props; | ||
|
|
||
| return ( | ||
| <EuiToolTip |
There was a problem hiding this comment.
May want consider passing down {...rest} to the tooltip so consumers can customize it even more and pass down something like 'data-test-subj'.
|
|
||
| let tooltip; | ||
| if (visible) { | ||
| if (visible && content) { |
There was a problem hiding this comment.
Sorry, one last thing, but because tooltips accept title's as well can you change this to be if (visible && (content || title))
5 participants
Kibana uses this in the home page tutorials and needs the component in am more generic location so it can be used in other places. Figured the best place for it is in EUI.