patternfly / patternfly-3

This repo contains the HTML, CSS, and JQuery for the PatternFly 3 project.
https://www.patternfly.org/v3
MIT License
1.15k stars 240 forks source link

Introduce Copy to Clipboard component #1112

Closed mcarrano closed 5 years ago

mcarrano commented 6 years ago

Copy to Clipboard has been added as a new pattern on the PatternFly website (https://www.patternfly.org/pattern-library/forms-and-controls/copy-to-clipboard). Need a corresponding Core implementation for this.

thebigtoona commented 6 years ago

I can look into implementing this :)

thebigtoona commented 6 years ago

Here's a link to the Codepen I'm working for this issue:

https://codepen.io/tmorozco/pen/vzmaPK?editors=0010

a couple of things - Its just the basic example right now and its still rough. I used a span instead of a button to maintain the design for the copy icon at the end, but I have been working on getting all the attributes and lables set properly in the html and jquery for accessibilty. I wrote this as if I were writing it to blend with bootstrap but if it isn't looking right please let me know. Any feedback would be awesome, community :) thanks!

mcoker commented 6 years ago

Hi @thebigtoona! This is looking good. re: the span vs. button, have you looked at using .input-group-btn to wrap the button in your input-group? Here is a fork of your pen with an example - https://codepen.io/mcoker/pen/EeOebe

This component (particularly the inline version) will likely be displayed within inline text (in the middle of a paragraph, for example). Currently the way it's structured, it resides within a form and is using bootstrap form classes. I might approach this as a more generic component that isn't bound to use inside of a form and can be displayed inline with text.

This component was also recently added to the patternfly-ng repo as an inline copy component and block copy component. I think your implementation matches the design specification better than the patternfly-ng component, but you can use that component as a reference for the general UI, interactivity, and accessibility.

thebigtoona commented 6 years ago

@michael-coker thank you for the feedback! I changed the span to resemble the one you showed me in the codepen. I also agree that it needs to be generic, that was a good point. right now it relies on value and id attributes from the source being copied from to grab the data. I will play around with it and see how it works inside of spans, divs, p elements etc and change the implementation accordingly.

thebigtoona commented 6 years ago

I got this working with a div element. but I had to work around execCommand('copy') by building an input on the fly, making it hidden, adding it to the dom and giving it the value of the text in the div element THEN manually selecting copying and removing the input from the DOM.

I haven't tested it yet but I think it will work with other general elements. I'm not sure if it will work with an input element yet I need to try that too or create a conditional for it. So far I haven't found a better way to do this with general elements. I came across some stuff that might be depreciated that approached it differently, and I wasn't able to get that working. I feel like this way is a little messy but I'd love to have some feedback on it.

This is the codepen: https://codepen.io/tmorozco/pen/YORMwB?editors=0010

UPDATE: I added a conditional for the input element so its known to work on both divs and inputs so far.

mcoker commented 6 years ago

Hey! Looking good! When I said make it more generic, I wasn't necessarily referring to the form elements (like <input>), but the form classes used to construct the layout. I think you're fine leaving the <input> inside of <div>s, so that you don't have to add/remove a DOM element just to execute the copy command. If you look at the inline copy example on patternfly-ng, there is an <input> there without being added to a form. I think that's fine, I would just not rely on bootstrap classes that need to be used in a form for the layout.

Also, looking at your class structure, instead of .copy-clipboard-container-pf, .copy-clipboard-text-pf, etc, on the outermost element, I would use .copy-clipboard-pf as the component's main class, then use classes like .copy-clipboard-pf-text, ..copy-clipboard-pf-btn, etc on the component element classes.

thebigtoona commented 6 years ago

@michael-coker thanks! I'm gonna look closer at the ng example and the code for that component. I'm not terribly familiar with angular, so I'm guessing that templated element in the html <pfng-inline-copy> is the where the input (and btn?) your talking about is located? I plan to pull it down and take a look at what's going on there for inspiration. Also, I agree with you on the classes and will change this.

thebigtoona commented 6 years ago

https://github.com/thebigtoona/patternfly-ng/blob/master/src/app/copy/copy-service/copy.service.ts

Ok, so I'm crawling the ng component and I think this is actually really similar to how I'm running the copy on my example, except with a textarea being appended/removed to/from the DOM instead of an input. This being the case I'm thinking I might need to leave my JS pretty similiar to what it currently is as far as approach goes but clean it up a bit.

I'm going to continue to look through this one for inspiration on the css too, because for things like the inline example you showed me I'm running into scrollbar issues with regard to Firefox (no webpack support) specifically and I haven't figured out how to get past that yet for cross compatibility.

LHinson commented 5 years ago

@thebigtoona we appreciate that you have taken the time to make a contribution to PatternFly! This feature request has gone stale so I'd like to close, however I wanted to reach out given your work on it. Let us know if you're interested in continuing to contribute to PatternFly, let's coordinate.