feat(clipboardcopy): add clipboardcopy to PF4 #1538

boaz0 · 2019-03-11T08:43:00Z

What:

closes #1417

Adding copy to clipboard into patternfly 4 react-core.

patternfly-build · 2019-03-11T09:04:29Z

PatternFly-React preview: https://1538-pr-patternfly-react-patternfly.surge.sh

codecov-io · 2019-03-11T09:14:41Z

Codecov Report

Merging #1538 into master will increase coverage by 0.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
+ Coverage   82.77%   82.79%   +0.01%     
==========================================
  Files         601      604       +3     
  Lines        6642     6655      +13     
  Branches       72       72              
==========================================
+ Hits         5498     5510      +12     
- Misses       1117     1118       +1     
  Partials       27       27

Flag	Coverage Δ
#patternfly3	`84.87% <ø> (ø)`	⬆️
#patternfly4	`79.53% <92.3%> (+0.06%)`	⬆️
#patternflymisc	`95.68% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...ct-core/src/components/ClipboardCopy/CopyButton.js	`100% <100%> (ø)`
...-core/src/components/ClipboardCopy/ToggleButton.js	`100% <100%> (ø)`
...re/src/components/ClipboardCopy/ExpandedContent.js	`80% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a42019...26c3de7. Read the comment docs.

dgutride · 2019-03-11T18:12:53Z

patternfly/patternfly#505 - here is the relevant patternfly core issue/design links

rachael-phillips · 2019-03-11T18:14:45Z

Related issue: #1417

tlabaj

@boaz0 can you update the description to include the related issue #1417

christiemolloy · 2019-03-13T20:49:25Z

@boaz0 I believe that when you click on the "copy to clipboard" button the popover should remain for 2 seconds approximately. @mcarrano are you able to check on this? Otherwise looks really good!

mcarrano · 2019-03-13T21:08:40Z

@boaz0 @christiemolloy Yes, the message that appears after you click Copy should stay for at least 2 seconds (maybe more?). We did not have a spec on this but I would try adding a 2 second timer and see how that feels.

Also, I noticed that in the Expandable version, the expansion area is not editable. It needs to be if a user is to be able to edit the entire contents. Also note that per the design. we should show the truncation ellipses when the text in the field gets truncated. See below.

tlabaj

Can you also add this the the react-integration app?

packages/patternfly-4/react-core/src/components/ClipboardCopy/ClipboardCopy.js

packages/patternfly-4/react-core/src/components/ClipboardCopy/CopyButton.d.ts

packages/patternfly-4/react-core/src/components/ClipboardCopy/copyFunction.js

packages/patternfly-4/react-core/src/components/ClipboardCopy/examples/ExpandedClipboardCopy.js

packages/patternfly-4/react-core/src/components/ClipboardCopy/ClipboardCopy.js

jgiardino · 2019-03-28T14:23:58Z

Great job on this, @boaz0! This looks great from an a11y perspective!

Yes, the message that appears after you click Copy should stay for at least 2 seconds (maybe more?). We did not have a spec on this but I would try adding a 2 second timer and see how that feels.

I agree that the tooltip should remain visible longer. For screen reader users, this pattern requires some updates to the tooltip so that the tooltip can function as an alert. This isn't a variation we support at the moment. I'll capture issues in core and react for this.

Also, I noticed that in the Expandable version, the expansion area is not editable. It needs to be if a user is to be able to edit the entire contents.

This is handled in core css. When I inspect the element, I don't see the overflow property that handles this in core. Maybe this was recently added to the core css and hasn't been pulled in yet?

tlabaj

LGTM

christiemolloy · 2019-03-28T20:39:09Z

@boaz0 I believe that when you click on the "copy to clipboard" button the popover should remain for 2 seconds approximately.

I had left this comment but it hasn't been implemented yet, could you try implementing the success tooltip to remain for 2 seconds?

christiemolloy · 2019-03-28T20:47:16Z

Also, when I type a lot of characters this happens, it might be something that we need on the Core side @mcoker do you know if we should add white-space: pre

boaz0 · 2019-03-31T11:59:32Z

@christiemolloy do you mind verifying that it is OK now. Thanks a lot

christiemolloy · 2019-04-01T15:27:34Z

@boaz0 Im still seeing this

boaz0 · 2019-04-01T15:33:14Z

It's going to be replaced by textarea. I will update it soon.

Thanks.

tlabaj

The way documentation is generated has been changed. Can you please remove the examples file. All examples should be in the md file.

boaz0 · 2019-04-15T13:50:31Z

@tlabaj yes sure.

tlabaj

LGTM

christiemolloy · 2019-04-15T18:07:42Z

A few comments:

In React there is no ... where the text gets cut off in the input group. This is how it should look:

Looks like the popover isn't directly in the center of the Copy to Clipboard icon

Also looks like that when the clipboard copy button is selected the popover doesnt remain for 2 seconds.

Signed-off-by: Boaz Shuster <[email protected]>

boaz0 · 2019-04-15T18:42:27Z

@christiemolloy

1 - We need to update the patternfly-next package to have that change.
2 - I believe it is related to #1740.
3 - Thanks for noticing, fixed that.

tlabaj

LGTM

mcoker · 2019-04-18T22:48:20Z

packages/patternfly-4/react-core/src/components/ClipboardCopy/ClipboardCopy.md

+import { ClipboardCopy } from '@patternfly/react-core';
+class SimpleClipboardCopy extends React.Component {
+  render() {
+    return <ClipboardCopy isReadOnly>This is editable</ClipboardCopy>;


Should this say "not editable" since it's read only?

Dang! You're right!

mcoker · 2019-04-18T23:36:42Z

What's the purpose of the <textarea>? It has inline styles, the font-size is too big, and it scrolls instead of the expandable content container, so the overflow and scrollbar look odd.

If we're going to keep that in the component, and the <textarea> should always wrap the expanded content, can we bring the element and the styling to core and fix the overflow/scrolling there, too?

If it was added to address the text wrapping issue @christiemolloy mentioned, we should be able to address that behavior in core, too, without the need for <textarea>.

boaz0 · 2019-04-22T20:36:55Z

@mcoker I added textarea because @mcarrano mentioned that the text should also be edited through the expanded content block.

Yep, you're right, I should have done it in core but I wanted to get feedback as soon as possible. I will take care of it if you don't mind.

Sorry for the mess.

Thanks.

mcoker · 2019-04-22T20:39:33Z

@boaz0 ah, I see! That's definitely a use case for a <textarea>. I also wonder if contenteditable on the HTMl element would work, which allows us to retain the styling and not need to introduce a new element?

boaz0 · 2019-04-22T20:43:41Z

@mcoker
First time I have heard of contenteditable - thanks.
Yes, it seems to work too. 👍

tlabaj added enhancement 🚀 labels Mar 11, 2019

tlabaj self-assigned this Mar 11, 2019

tlabaj requested review from tlabaj and kmcfaul March 11, 2019 15:37

tlabaj reviewed Mar 11, 2019

View reviewed changes

tlabaj added ux review labels Mar 12, 2019

tlabaj requested a review from christiemolloy March 12, 2019 19:19

tlabaj added css review and removed css approved labels Mar 12, 2019

dlabaj previously approved these changes Mar 19, 2019

View reviewed changes

tlabaj added the A11y label Mar 21, 2019

tlabaj requested a review from jgiardino March 21, 2019 14:57

tlabaj requested changes Mar 21, 2019

View reviewed changes

boaz0 dismissed dlabaj’s stale review via 8955412 March 21, 2019 20:54

tlabaj reviewed Mar 27, 2019

View reviewed changes

packages/patternfly-4/react-core/src/components/ClipboardCopy/ClipboardCopy.js Outdated Show resolved Hide resolved

jgiardino mentioned this pull request Mar 28, 2019

Tooltip variation - announce contents as alert #1655

Closed

tlabaj previously approved these changes Mar 28, 2019

View reviewed changes

dlabaj self-assigned this Mar 29, 2019

tlabaj unassigned dlabaj Mar 29, 2019

boaz0 dismissed tlabaj’s stale review via 41268b1 March 31, 2019 11:58

tlabaj requested changes Apr 15, 2019

View reviewed changes

tlabaj previously approved these changes Apr 15, 2019

View reviewed changes

feat(clipboardcopy): add clipboardcopy to PF4

26c3de7

Signed-off-by: Boaz Shuster <[email protected]>

boaz0 dismissed tlabaj’s stale review via 26c3de7 April 15, 2019 18:38

christiemolloy approved these changes Apr 18, 2019

View reviewed changes

tlabaj approved these changes Apr 18, 2019

View reviewed changes

mcoker reviewed Apr 18, 2019

View reviewed changes

tlabaj merged commit 6a5305e into patternfly:master Apr 22, 2019

boaz0 deleted the clipboardcopy_pf4 branch April 24, 2020 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clipboardcopy): add clipboardcopy to PF4 #1538

feat(clipboardcopy): add clipboardcopy to PF4 #1538

boaz0 commented Mar 11, 2019 •

edited

Loading

patternfly-build commented Mar 11, 2019

codecov-io commented Mar 11, 2019 •

edited

Loading

dgutride commented Mar 11, 2019

rachael-phillips commented Mar 11, 2019

tlabaj left a comment

christiemolloy commented Mar 13, 2019

mcarrano commented Mar 13, 2019

tlabaj left a comment

jgiardino commented Mar 28, 2019

tlabaj left a comment

christiemolloy commented Mar 28, 2019

christiemolloy commented Mar 28, 2019

boaz0 commented Mar 31, 2019

christiemolloy commented Apr 1, 2019

boaz0 commented Apr 1, 2019

tlabaj left a comment

boaz0 commented Apr 15, 2019

tlabaj left a comment

christiemolloy commented Apr 15, 2019

boaz0 commented Apr 15, 2019 •

edited

Loading

tlabaj left a comment

mcoker Apr 18, 2019

boaz0 Apr 22, 2019

mcoker commented Apr 18, 2019 •

edited

Loading

boaz0 commented Apr 22, 2019 •

edited

Loading

mcoker commented Apr 22, 2019

boaz0 commented Apr 22, 2019

feat(clipboardcopy): add clipboardcopy to PF4 #1538

feat(clipboardcopy): add clipboardcopy to PF4 #1538

Conversation

boaz0 commented Mar 11, 2019 • edited Loading

patternfly-build commented Mar 11, 2019

codecov-io commented Mar 11, 2019 • edited Loading

Codecov Report

dgutride commented Mar 11, 2019

rachael-phillips commented Mar 11, 2019

tlabaj left a comment

Choose a reason for hiding this comment

christiemolloy commented Mar 13, 2019

mcarrano commented Mar 13, 2019

tlabaj left a comment

Choose a reason for hiding this comment

jgiardino commented Mar 28, 2019

tlabaj left a comment

Choose a reason for hiding this comment

christiemolloy commented Mar 28, 2019

christiemolloy commented Mar 28, 2019

boaz0 commented Mar 31, 2019

christiemolloy commented Apr 1, 2019

boaz0 commented Apr 1, 2019

tlabaj left a comment

Choose a reason for hiding this comment

boaz0 commented Apr 15, 2019

tlabaj left a comment

Choose a reason for hiding this comment

christiemolloy commented Apr 15, 2019

boaz0 commented Apr 15, 2019 • edited Loading

tlabaj left a comment

Choose a reason for hiding this comment

mcoker Apr 18, 2019

Choose a reason for hiding this comment

boaz0 Apr 22, 2019

Choose a reason for hiding this comment

mcoker commented Apr 18, 2019 • edited Loading

boaz0 commented Apr 22, 2019 • edited Loading

mcoker commented Apr 22, 2019

boaz0 commented Apr 22, 2019

boaz0 commented Mar 11, 2019 •

edited

Loading

codecov-io commented Mar 11, 2019 •

edited

Loading

boaz0 commented Apr 15, 2019 •

edited

Loading

mcoker commented Apr 18, 2019 •

edited

Loading

boaz0 commented Apr 22, 2019 •

edited

Loading