feat(CopyToClipboard): add prototype #1860
Conversation
lucivpav
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1860 +/- ##
======================================
Coverage 69.9% 69.9%
======================================
Files 888 888
Lines 7775 7775
Branches 2244 2271 +27
======================================
Hits 5435 5435
Misses 2330 2330
Partials 10 10Continue to review full report at Codecov.
|
| })} | ||
| /> | ||
| {hideContent && ( | ||
| <Portal |
There was a problem hiding this comment.
Let's extract this to a separate component like CopyButtonNotification. We can reuse our createComponent() to host styles.
const CopyButtonNotification = createComponent({
displayName: 'CopyButtonNotification',
render: () => {
return (
<Portal>
<div className={classes.root}>
<div className={classes.content}>{children}</div>
</div>
</Portal>
)
},
})
There was a problem hiding this comment.
Agree with createComponent, also consolidating styles in the provider or theme would be nice.
Would it be possible to create a "Notification" component instead so that you can use without the button? (which I believe Shift's example might be showing as well)
There was a problem hiding this comment.
Would it be possible to create a "Notification" component instead so that you can use without the button?
I think so 👍
| const { attached, pointing, copyPrompt } = this.props | ||
| const hideContent = this.state.copied && !attached | ||
| const copiedStr = 'Copied to clipboard' | ||
| const copiedText = hideContent ? ( |
There was a problem hiding this comment.
I am not fully understand this logic, can you please clarify?
I expect something like this:
{attached && copied && <CopyButtonNotification>{notice}</CopyButtonNotification>}
{!attached && <Tooltip>{copied ? notice : usualMessage}</CopyButtonNotification>}There was a problem hiding this comment.
We don't want to show the window-centered message when we are in attached mode. In attached mode, we just change the content of the Tooltip.
And to make the centered message more visible, I'm setting Text's size to larger.
There was a problem hiding this comment.
hideContent implies that we should hide the Tooltip's content, as instead we are displaying the centered message.
| ...(hideContent && hiddenPopupContentVariables), | ||
| }, | ||
| }} | ||
| on={['hover', 'context']} |
There was a problem hiding this comment.
I tried the prototype - it shows the message correctly after click, but then when I move my cursor outside of the button and return back to it, it shows the message again - I believe it's because of 'hover'. I think it should be on={[]}. Not sure if Toolbar would behave better or worse
There was a problem hiding this comment.
This is intentional. Upon clicking the button, the components gets into a copied state. During the duration of this state, I'm showing 'Copied to clipboard' message. Would you prefer to show the original "Copy commit ID" message instead?
There was a problem hiding this comment.
the message should be visible also when you do not hover/focus on the button.
There was a problem hiding this comment.
It is now visible also when the button is not hovered/focused
Props
Examples
Issues
When clicking the not attached version, use has no way to interact with elements other than the message.