feat: Make it so PFE-Clipboard can copy arbitrary content #1795

@bennypowers that'd be awesome! 🤔 Though I don't know that there's any use case for it yet.
I want this PR for a specific feature in my main project, if we think adding that would cause this PR to take longer before it's merged, I'd prefer we did it in a separate PR.

elements/pfe-clipboard/src/pfe-clipboard.js

elements/pfe-clipboard/demo/index.html

…ithub.com:patternfly/patternfly-elements into 1794-allow-pfe-clipboard-to-copy-arbitrary-things

wesruv · 2021-10-15T17:57:58Z

EDIT
Nevermind the question in this comment, see below comment, discussed this with the DXP CPFED folks.

The code is good to go as far as I'm concerned, but I it still needs docs. Wanted to bounce this issue off folks before I wrote the docs:

I added a few disabled states for when:

target='property' and el.contentToCopy is not set
- I added a getter and setter so I don't think there's a chance the component will be disabled if that value is set
EDIT No longer doing this /EDIT target='selector' and element in selector can't be found
- This one is not fool proof. The code checks to see if the selector finds an element on connectedCallback, on DOMContentLoaded, and load, but if the content is injected dynamically the site would have to call el.checkForCopyTarget()
- It also runs checkForCopyTarget() when the target attribute changes, which helps with the issue in the previous bullet

The only other ways of checking to see if the element exists would be heavy... like a MutationObserver on the document 🤢 ...

Do you all think this is good enough? Is there a 'cool kid' way to handle that 'not fool proof' concern that I'm missing?

Discussed with Kyle Buchanan, Michael Clayton, and other CPFED folks, guarding against the HTML element that will have it's text copied isn't really worth it. There isn't a good way to know that it _should_ be disabled and then _should_ be enabled later. Better to leave it to the click event, and hopefully things were setup so it'll work when the user clicks on it

wesruv · 2021-10-15T18:48:26Z

The code is good to go as far as I'm concerned, but I it still needs docs. Wanted to bounce this issue off folks before I wrote the docs:
...
Do you all think this is good enough? Is there a 'cool kid' way to handle that 'not fool proof' concern that I'm missing?

Discussed this with Kyle Buchanan, Michael Clayton, and other CPFED folks, and it feels like checking to see if an element exists isn't worth it. We can know if the property is set, but if an element exists is too hairy, and not a necessary thing to guard against.

coreyvickery · 2021-10-15T19:51:38Z

@heyMP Please ensure these colors are integrated or let me know if there is another issue where I can put this information.

Light theme

Icon color: #6a6e73
Hover color: #151515

Dark theme

Icon color: #d2d2d2
Hover color: #fff

heyMP · 2021-10-15T19:59:09Z

@coreyvickery sounds good.

@wesruv I'm going to add these styles to the icon quick if that's ok?

wesruv · 2021-10-18T16:16:52Z

@wesruv I'm going to add these styles to the icon quick if that's ok?

Sounds great, thanks sir!

elements/pfe-clipboard/src/pfe-clipboard.js

heyMP · 2021-10-19T12:05:49Z

@corey Here are the icon changes:
https://deploy-preview-1795--patternfly-elements.netlify.app/elements/pfe-clipboard/demo/

Light theme Icon color: #6a6e73; Hover color: #151515
Dark theme Icon color: #d2d2d2; Hover color: #fff

I also added a saturated theme where I used the following variables. I tried to semantically match them to their corresponding dark theme variables but they can be changed.

icon color: text--muted--on-saturated: #d2d2d2
icon color hover: text--on-saturated #fff

coreyvickery · 2021-10-19T14:46:59Z

@heyMP Looks great.

heyMP · 2021-10-19T15:56:01Z

elements/pfe-clipboard/src/pfe-clipboard.scss

+// Attach variables to hover, focus states
+// ignore disabled.
+:host(:hover:not([disabled])),
+:host(:focus:not([disabled])) {
+  .pfe-clipboard {
+    &__text {
+      color: pfe-local(Color--hover) !important;
+    }
+    &__icon {
+      // Customize icon color for pfe-icons
+      --pfe-icon--Color: #{pfe-local(icon--Color--hover)};
+      svg {
+        // Customize icon color of fallback svg icon
+        fill: pfe-local(icon--Color--hover) !important;
+      }
+    }
+  }
+}
+


Simplified our old focus,hover selectors.

heyMP · 2021-10-19T15:57:18Z

elements/pfe-clipboard/src/pfe-clipboard.scss

+:host([on="dark"]) {
+  --pfe-clipboard--icon--Color: #{pfe-local(icon--Color--dark)};
+  --pfe-clipboard--icon--Color--hover: #{pfe-local(icon--Color--hover--dark)};
 }

-// Note: Hover states need to be defined after focus states
-:host(:not([aria-disabled="true"]):hover),
-:host(:not([aria-disabled="true"])) ::slotted(:hover) {
-  --pfe-clipboard--Color: #{pfe-local(Color--hover)};
+:host([on="saturated"]) {
+  --pfe-clipboard--icon--Color: #{pfe-local(icon--Color--saturated)};
+  --pfe-clipboard--icon--Color--hover: #{pfe-local(icon--Color--hover--saturated)};


Explicitly adding theme context hooks for icon color. We don't have to include text color variables because that's automatically covered by broadcast variables.

heyMP · 2021-10-19T16:02:11Z

@wesruv I think almost ready. Sorry, this review took a while; but we have some really great improvements!

I think we'll just need to add this new copy-from API added to our docs and then we're good to go! 😁

wesruv · 2021-10-19T16:28:45Z

Removed those unnecessary removeEventListeners

This reverts commit 8de71ec.

heyMP · 2021-10-27T21:35:07Z

Hey, @wesruv. Would you be able to add the new "copy-from" attribute API to the docs?

heyMP

LGTM!

github-actions bot added demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. labels Oct 8, 2021

Added proof of concept for copying arbitrary content

23a2c23

Need docs update and tests still

wesruv force-pushed the 1794-allow-pfe-clipboard-to-copy-arbitrary-things branch from 4e17be8 to 23a2c23 Compare October 8, 2021 18:15

wesruv commented Oct 8, 2021

View reviewed changes

elements/pfe-clipboard/src/pfe-clipboard.js Outdated Show resolved Hide resolved

wesruv commented Oct 8, 2021

View reviewed changes

elements/pfe-clipboard/demo/index.html Outdated Show resolved Hide resolved

wesruv commented Oct 8, 2021

View reviewed changes

elements/pfe-clipboard/demo/index.html Show resolved Hide resolved

github-actions bot added the AT passed Automated testing has passed label Oct 8, 2021

heyMP and others added 5 commits October 13, 2021 15:39

ported pfe-clipboard tests to web-test-runner

0ec75d4

removed old slotchange event

a54d78b

Branch was auto-updated with the latest.

1a505bc

removed arrow functions to allow individual tests to override timeouts.

de204c9

Merge pull request #1800 from patternfly/1794-allow-pfe-clipboard-to-…

5079de5

…copy-arbitrary-things--new-tests feat: Update tests for pfe-clipboard

github-actions bot added the tests Related to testing label Oct 14, 2021

heyMP reviewed Oct 14, 2021

View reviewed changes

elements/pfe-clipboard/src/pfe-clipboard.js Show resolved Hide resolved

elements/pfe-clipboard/src/pfe-clipboard.js Outdated Show resolved Hide resolved

elements/pfe-clipboard/demo/index.html Outdated Show resolved Hide resolved

wesruv added 2 commits October 15, 2021 10:53

Adding disabled states for when copy target has issues

9e04769

Merge branch '1794-allow-pfe-clipboard-to-copy-arbitrary-things' of g…

534a6d8

…ithub.com:patternfly/patternfly-elements into 1794-allow-pfe-clipboard-to-copy-arbitrary-things

github-actions bot added the styles An issue or PR pertaining only to CSS/Sass label Oct 15, 2021

Writing tests to for disabled state and fixing resulting bugs

8ac5602

wesruv changed the title ~~WIP: Added proof of concept for copying arbitrary content~~ feat: Added proof of concept for copying arbitrary content Oct 15, 2021

wesruv added 2 commits October 15, 2021 13:04

Cleaning up code a bit

b0479b2

Added docs and fixed some bugs

5adf5dc

github-actions bot added the docs Documentation updates label Oct 15, 2021

On element copy, use innerText instead of innerHTML, think it's safer

d4b70c1

wesruv changed the title ~~feat: Added proof of concept for copying arbitrary content~~ feat: Make it so PFE-Clipboard can copy arbitrary content Oct 15, 2021

added correct icon colors

f3bcb5a

heyMP reviewed Oct 19, 2021

View reviewed changes

elements/pfe-clipboard/src/pfe-clipboard.js Outdated Show resolved Hide resolved

ignore [disabled] on focus,hover states

96c3138

update docs

808b903

heyMP reviewed Oct 19, 2021

View reviewed changes

Removing unnecessary event listener ... removal.

5258c5f

heyMP added 2 commits October 27, 2021 16:43

update docs

8de71ec

Revert "update docs"

04759d7

This reverts commit 8de71ec.

github-actions bot and others added 4 commits October 29, 2021 16:01

Branch was auto-updated with the latest.

2042b70

Branch was auto-updated with the latest.

2af2965

Providing better default UI for buttons not copying a URL

985d45e

Updating documentation for pfe-clipboard based on updates

e2dfcdc

wesruv force-pushed the 1794-allow-pfe-clipboard-to-copy-arbitrary-things branch from 4fe2065 to e2dfcdc Compare November 5, 2021 17:26

wesruv and others added 2 commits November 5, 2021 12:40

Updating default content for icon slot in tests

b573beb

chore: update docs

d83a5c5

heyMP approved these changes Nov 5, 2021

View reviewed changes

github-actions bot added the ready to merge label Nov 5, 2021

heyMP merged commit b15c476 into master Nov 5, 2021

heyMP deleted the 1794-allow-pfe-clipboard-to-copy-arbitrary-things branch November 5, 2021 19:43

bennypowers mentioned this pull request Mar 21, 2022

PFE-Clipboard: Copy more than the current URL #1794

Closed

feat: Make it so PFE-Clipboard can copy arbitrary content #1795

feat: Make it so PFE-Clipboard can copy arbitrary content #1795

Uh oh!

Conversation

wesruv commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Uh oh!

Uh oh!

netlify bot commented Oct 8, 2021

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bennypowers commented Oct 11, 2021

Uh oh!

wesruv commented Oct 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wesruv commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesruv commented Oct 15, 2021

Uh oh!

coreyvickery commented Oct 15, 2021

Light theme

Dark theme

Uh oh!

heyMP commented Oct 15, 2021

Uh oh!

wesruv commented Oct 18, 2021

Uh oh!

Uh oh!

heyMP commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreyvickery commented Oct 19, 2021

Uh oh!

heyMP Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

wesruv Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

wesruv Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP commented Oct 19, 2021

Uh oh!

wesruv commented Oct 19, 2021

Uh oh!

heyMP commented Oct 27, 2021

Uh oh!

heyMP left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wesruv commented Oct 8, 2021 •

edited

Loading

netlify bot commented Oct 8, 2021 •

edited

Loading

wesruv commented Oct 15, 2021 •

edited

Loading

heyMP commented Oct 19, 2021 •

edited

Loading