feat(clipboard-copy): add inline/block level copy component, copy service #351

seanforyou23 · 2018-05-17T02:55:02Z

This PR adds two new components, and a service. The two components are an inline and block level variation of a "copy value to clipboard" functionality. These components are simple, flexible, and accessible. They are also WIP! The basic functionality is there, feel free to test drive and make suggestions!

https://rawgit.com/seanforyou23/patternfly-ng/copy-clipboard-dist/dist-demo/
https://rawgit.com/seanforyou23/patternfly-ng/copy-clipboard-dist/dist-demo/#/inlinecopy
https://rawgit.com/seanforyou23/patternfly-ng/copy-clipboard-dist/dist-demo/#/blockcopy
https://rawgit.com/seanforyou23/patternfly-ng/copy-clipboard-dist/dist-demo/#/copyservice

create block-level copy component
create inline copy component
create copy module
create copy service
add new components and service to demo navigation
accessibility considerations
add unit tests for new components and service
copy service examples
label is only required input prop
support multiple copy components per view
ensure semantic relation between label and copy button
use button instead of repurposing icon element
add tooltips for hover state
examples that use callbacks to launch toast notifications
tslint rules
expanded
dedicated tooltip and aria label inputs
clean up examples
export * as modules
align markup between inline and block versions
basic, expanded, wrap, multiple, and callback examples

dlabrecq

Built/ran the demo locally. Looks nice, just minor comments

dlabrecq · 2018-05-17T14:32:20Z

src/app/copy/block-copy/block-copy.component.less

@@ -0,0 +1,55 @@
+.copy {


We should prefix patternfly-ng selectors with pfng-, unless this is extending an existing patternfly selector?

dlabrecq · 2018-05-17T14:34:26Z

src/app/copy/inline-copy/inline-copy.component.less

@@ -0,0 +1,31 @@
+.inline-copy {


We should prefix this with pfng-

dlabrecq · 2018-05-17T14:36:09Z

src/app/copy/inline-copy/inline-copy.component.ts

+})
+
+export class InlineCopyComponent implements OnInit {
+  @Input('label') label: string;


The label name is misleading since it appears to be used for the aria-label. I thought this was for the button label until I looked at the HTML template.

Can we rename this as ariaLabel? We may want to add a unique tooltip input as well, considering the tooltip and aria-label are different.

Sure I see, I was using label here because it aligned nicely with the same label input property for the block level copy component but it's a good point about it not actually being label text, I'll update this.

Allowing for custom tooltip text sounds like a good idea, I'll just set it to the ariaLabel value by default, and users can override if they like.

I don't believe either should be required. If a tooltip is not provided, lets not output one.

dlabrecq · 2018-05-17T14:37:18Z

src/app/copy/inline-copy/inline-copy.component.ts

+
+export class InlineCopyComponent implements OnInit {
+  @Input('label') label: string;
+  @Input('token') token: string = 'Missing \'token\' @Input property';


I imagine users will want to copy more than tokens? Can we rename this as value or perhaps copyValue?

I was thinking of "token" in the generic sense like "node" or something. copyValue is nice and obvious :)

dlabrecq · 2018-05-17T14:42:07Z

src/app/copy/inline-copy/inline-copy.component.ts

+export class InlineCopyComponent implements OnInit {
+  @Input('label') label: string;
+  @Input('token') token: string = 'Missing \'token\' @Input property';
+  @Input('copyBtnTxt') copyBtnTxt: string = 'Copy';


Can we rename this as buttonLabel? That would be more consistent with other components.

src/app/copy/block-copy/block-copy.component.html

dlabrecq · 2018-05-17T15:41:17Z

src/app/copy/block-copy/block-copy.component.html

+      </div>
+
+      <button
+        [attr.id]="'pf-copy-token-button-' + hash"


Is the hash being used to create a unique ID? If so, we should talk about standardizing how we do that for patternfly-ng components. We may want to allow the application to provide its own ID.

FYI, there is an issue to provide checkbox IDs for pfng-list. Need to make sure whatever we do here is consistent.

Ah, yes good call. I figured that would come up and I'm all for it. You know, though, I'd really rather not use a arbitrary hash here of at all, if possible, and instead use an actual text string that expresses some semantic meaning. For example mapping over the aria label (or something like that) and appending a dash-cased-version of that to the ID. That could be really long so maybe not that specifically, but you get the idea.

I really just couldn't think of anything I liked while I was writing that so I sort of stubbed it out with this thingie.

You know, this brings up another thing I wanted to point out, which is that because the button and label are linked, if you select the label, it's the same as clicking the button. This seems on the surface like a nice thing to have, but wanted to point it out in case this causes unintended tripping hazard/frustration like "oops I didn't mean to copy, now I'm upset".

Not so big a deal for the sighted user, but for keyboard user, we have no idea how difficult/time consuming it may be to retrieve whatever value they previously had in their clipboard. I could use a span instead of label to remove this interactivity and keep the visual appearance. So that's another option.

If the act of selecting the label causing copy to clipboard action is problematic, we could get rid of the id altogether.

The original design didn't have a label, but I felt like it needed one during implementation because with more than one on the screen, you need a way to ENSURE there is some difference between the buttons, otherwise they all show up as "Copy button" or something similar depending on reader.

Dropping the visual label and instead opting for an aria-label in it's place could be another option.

If we go for the hash based approach, let's use an incrementing counter over random string?

@dongniwang or @jgiardino may know of a tie breaker. Definitely worth checking if there's a single solution for both this and pfng-list.

Can you describe the user flow you're asking about? I'm not really following what the behavior in question is.

Although while trying to figure it out, I did notice that when I have focus on a Copy button and hit Enter, the focus isn't maintained on the Copy button. I would expect that focus to remain on the action I selected unless there's a reason to shift focus.

Sure, so the flow would be; a user clicks on the text label for the block copy component, which causes the text to be copied to their clipboard.

In general, for form controls, it's expected that when a user clicks the label, it will result in the same behavior as if the user clicks the corresponding form control directly. The more common scenario is when a user clicks a label resulting in focus placed in a related input. With the related control being a button, though, the result is whatever happens when you click the button.

Would this be considered unexpected behavior? I'm starting to think it may be, but wanted another opinion. It's easy to make this work either way, just wondering what's best.

Noticed the focus leaving the button on keypress, I'll fix this. Good catch.

My concern is more about how do we generate IDs for patternfly-ng overall. I'm thinking if the user provides an ID, we should use that as a prefix. Lodash has a uniueid method we could use by default if an ID isn't given.

If we were to use a provided "id" as a prefix, then it becomes, well, not the id anymore. I'm wondering what value this would add from a users perspective. It shouldn't really matter since it's a generated id anyway, right? I could be missing something..

I'm starting to think uniqueId primed with "pfng-block-copy-button-" does the job well enough and is inline with the the work you did over in list recently - #360

One caveat - I appended an extra dash to the prefix in my latest changes because I think it looks nice with the counter being separated with a dash like the rest of the string, but I could be convinced otherwise :)

@dlabrecq let me know what you think, happy to provide a user defined id prefix, just really wanna understand what this might be used for before I put it in there. Thx!

Being able to provide an ID allows users to write tests based on something that is known to them. For example, if someone wrote a Selimium test to activate the button, the test would need to rely on a consistent ID. A unique, generated ID isn't something a test can rely on because it is always different. Using a generated ID is fine by default, but we need to allow for that to be overridden. The convention used by the components should be consistent -- please follow #360.

dlabrecq · 2018-05-17T15:46:09Z

src/app/copy/copy-service/copy.service.spec.ts

+  });
+
+  it('should return false if the dom copy command has failed', () => {
+    spyOn(service.dom, 'execCommand').and.throwError('expected-test-error');


Are we throwing an error or writing to the console? Or, both?

Looks like the service is catching that...

Yea I think that description is from copy pasta, good catch! I'll clean this up with the rest of the tests

src/app/copy/inline-copy/inline-copy.component.less

dlabrecq

Additional comments are hidden above...

jgiardino · 2018-05-18T00:16:51Z

These examples look great! I love that you have aria-label on the icon buttons 👍

<button class="copy-btn" aria-label="Copy Example Sparql Query">
    <i class="fa fa-copy"></i>
  </button>

But since the icon provides no meaning to a screen reader, you can also add aria-hidden="true":
<i class="fa fa-copy" aria-hidden="true"></i>

seanforyou23 · 2018-05-19T15:44:11Z

Thanks for the awesome feedback everyone, I've addressed a good bit of it but still have some cleaning up to do, particularly around the examples and tests - but definitely a little more component work as well. More feedback is welcome while I'm working through those things though!

dlabrecq

Looking good. I know this is still a WIP, but here is some initial feedback.

I also found a small CSS issue -- missing border when expanded.

src/app/copy/block-copy/block-copy.module.ts

dlabrecq · 2018-05-21T15:39:04Z

src/app/copy/block-copy/block-copy.module.ts

+import { CommonModule } from '@angular/common';
+import { NgModule } from '@angular/core';
+
+import { TooltipConfig, TooltipModule } from 'ngx-bootstrap/tooltip';


TooltipConfig is unused. We need to add TooltipConfig as a provider, unless we don't really need a tooltip here?

If you do that here, it may not be required in the example? If that can be removed from the examples, all the better.

Good call, I've been having some issues with tooltips and this may be part of it. Checking into it now, thx!

dlabrecq · 2018-05-21T15:43:31Z

src/app/copy/block-copy/block-copy.component.html

+<div class="pfng-block-copy">
+  <label
+    class="pfng-block-copy-label"
+    [attr.for]="'pfng-block-copy-value-button-' + hash">{{label}}</label>


If the label has not been provided, I'd like to make this optional (i.e., add an *ngIf statement here).

dlabrecq · 2018-05-21T15:45:47Z

src/app/copy/block-copy/block-copy.component.ts

+  }
+
+  ngOnInit() {
+    if (!this.label) throw new Error('Missing required @Input property \'label\'');


The label isn't something that should be required.

Let's not throw an error here, but provide defaults for copyBtnAriaLabel and expandBtnAriaLabel instead. Alternatively, we can use the button label, if provided?

The button label is most likely to always be used, but neither should throw an error.

Good suggestions - I'm with ya. It's sort of where I was headed initially when I was requiring the label, and using that to construct unique aria label defaults. For screen readers, it's important for the aria labels to be unique per component because if they are not, and there are more than one in a single page, it's impossible to tell the copy/expand buttons apart from one another.

I'm starting to think it's a problem we shouldn't try to solve for people at the component level, and instead just leave the component in a most flexible state. It doesn't ensure the component will be "accessible" in all scenarios, but it may be a reservation we have to make.

I'll keep thinking about this, in the mean time I'll go ahead and push my next set of updates.

dlabrecq · 2018-05-21T15:48:34Z

src/app/copy/block-copy/block-copy.component.ts

+  /**
+   * Used to uniquly relate label to copy button
+   */
+  public _hash: number = Math.floor(Math.random() * 10000);


Can we use the uniqueid method from lodash?

dlabrecq · 2018-05-21T16:03:11Z

src/app/copy/inline-copy/inline-copy.component.ts

+export class InlineCopyComponent implements OnInit {
+  @Input('ariaLabel') ariaLabel: string;
+  @Input('copyValue') copyValue: string = 'Missing \'copyValue\' @Input property';
+  @Input('tooltipTxt') tooltipTxt: string;


Please rename this to tooltip.

We may also want to provide a tooltipPlacement input so devs can specify how it's shown. We don't always have room at the top.

dlabrecq · 2018-05-21T16:04:16Z

src/app/copy/inline-copy/inline-copy.component.html

+  <span
+    class="pfng-inline-copy-txt-cont"
+    data-toggle="tooltip"
+    data-placement="top"


data-toggle and data-placement are old attributes. Please use the ngx-bootstrap attributes; placement and tooltip.

I didn't realize those where old, good catch! I was a little hesitant to use placement/tooltip at first because they are not valid html5 attributes and are reported as error in tools like markup validation service - https://validator.w3.org/, but to be fair it's really just a conformance rule that everyone breaks anyway. I'll go with the ngx-bootstrap attributes for now.

dlabrecq · 2018-05-21T16:05:36Z

src/app/copy/inline-copy/inline-copy.component.html

+    class="pfng-inline-copy-txt-cont"
+    data-toggle="tooltip"
+    data-placement="top"
+    [attr.tooltip]="tooltipTxt">


If you're using the HTML tooltip attribute, we don't need the ngx-bootstrap functionality. However, I'm thinking we want to use ngx-bootstrap for consistency.

dlabrecq · 2018-05-21T16:07:16Z

src/app/copy/inline-copy/inline-copy.component.ts

+  ngOnInit(): void {
+    if (!this.ariaLabel) throw new Error('Missing required @Input property \'ariaLabel\'');
+    if (!this.tooltipTxt) {
+      this.tooltipTxt = this.ariaLabel;


Please do not throw an error here for ariaLabel. The ariaLabel should be optional or we can set a default.

We should also not set a tooltip by default.

dlabrecq · 2018-05-21T16:10:09Z

src/app/copy/block-copy/block-copy.component.html

+          class="pfng-block-copy-preview-txt"
+          data-toggle="tooltip"
+          data-placement="top"
+          [attr.tooltip]="tooltip">{{copyValue}}</span>


We're using both ngx-bootstrap and HTML tooltip attributes -- we only need one or the other.

data-toogle and data-placement should be replaced with ngx-bootstrap attributes; placement and tooltip.

See: https://ngx-bootstrap.surge.sh/#/tooltip

seanforyou23 · 2018-05-23T21:53:56Z

Awesome feedback everyone. I've just pushed another round of updates, I know there's some cleanup to do regarding tests, but it's ready for another round of review whenever anyone finds the time! Thx in advance.

mcoker · 2018-05-24T14:47:44Z

src/app/copy/inline-copy/inline-copy.component.less

+  &-txt-cont {
+    flex-grow: 1;
+    display: inline;
+    align-items: center;


display: inline; looks unnecessary .pfng-inline-copy-txt-cont and align-items: center; doesn't do anything since it isn't a flex parent.

mcoker · 2018-05-24T14:52:33Z

src/app/copy/block-copy/block-copy.component.less

+  }
+  &-preview {
+    display: flex;
+    align-items: stretch;


align-items: stretch; is the default value for align-items so that can be removed.

mcoker · 2018-05-24T15:00:50Z

This looks great @seanforyou23!!

One bit of feedback about the block level copy example - have you considered adding a right border/divider between the arrow on the left and the text to copy? Currently it kinda looks like the arrow is part of the text that will be copied.

dlabrecq · 2018-05-30T15:29:27Z

src/app/copy/block-copy/block-copy.component.ts

+  @Input('buttonLabel') buttonLabel: string = 'Copy';
+  @Input('expanded') expanded: boolean = false;
+
+  public uniqueID: string = uniqueId('pfng-block-copy-button-');


I'd like to make this equal to uniqueId('pfng-block-copy');. That way, lodash's unique ID is not appended to the end of the string.

See below...

dlabrecq · 2018-05-30T15:34:05Z

src/app/copy/block-copy/block-copy.component.ts

+   * Used to uniquly relate label to copy button
+   */
+  get copyBtnId(): string {
+    return this.uniqueID;


Then, this can return this.uniqueID + '-button';

The resulting ID would look more like pfng-block-copy1-button. And anywhere else we need an ID later, we could create something like pfng-block-copy1-arialabel, for example.

This makes testing easier if each ID is prefixed similarly. This would also follow the convention in pfng-list.

dlabrecq · 2018-05-30T15:35:03Z

src/app/copy/block-copy/block-copy-config.ts

@@ -0,0 +1,27 @@
+import { CopyConfigBase } from '../copy-config-base';


Considering the components are using inputs, this appears to be unused?

I'd like to remove the config classes and move the typedoc comments to each component input.

So the thought here was to specify these shared values as part of a config class to ensure consistency in the names/types of component inputs. It was nice to be able to import this class in the tests and example files for both copy components as it sort of guided the development experience. I suppose the value here isn't has big as I first thought. I'll remove this config class and redefine these inputs for each of the copy components.

That's an interesting idea and I did give that some thought. However, considering we have the same import properties on the component, it's extra APIs and typedoc to maintain. The fewer public APIs the better -- more difficult to deprecate later.

Going forward, I'd like to deprecate the config objects we have today in favor of component imports. It's easier for developers to bind properties directly to component tags rather than setup a config object in the view controller.

That said, we will likely keep some config objects for third party libraries. For example, using a config object to specify only C3 properties may still be helpful for chart components.

Thanks for the clarification, that sounds like a great strategy.

dlabrecq · 2018-05-30T15:37:39Z

src/app/copy/block-copy/block-copy.component.ts

+  @Input('label') label: string;
+  @Input('expandBtnAriaLabel') expandBtnAriaLabel: string;
+  @Input('buttonLabel') buttonLabel: string = 'Copy';
+  @Input('expanded') expanded: boolean = false;


Please add doc comments for each input.

dlabrecq · 2018-05-30T15:50:38Z

src/app/copy/block-copy/example/block-copy-callback-example.component.ts

+  selector: 'block-copy-callback-example',
+  templateUrl: './block-copy-callback-example.component.html'
+})
+export class BlockCopyCallbackExampleComponent implements OnInit {


Can we rename this example as "notification" Vs "callback"?

It was a little confusing to see the word "callback" in the example. Callbacks were something heavily used in Angular Patternfly. Instead of invoking user provided functions, we've been using events in patternfly-ng.

Considering the example is showing a notification, that may be more appropriate?

src/app/copy/inline-copy/inline-copy-config.ts

dlabrecq · 2018-05-30T15:56:18Z

src/app/copy/inline-copy/inline-copy.component.ts

+    super(copyService);
+  }
+
+  copyValueToClipboard(): void {


Can this method be marked protected?

dlabrecq · 2018-05-30T15:57:01Z

src/app/copy/copy-base.ts

+  @Input('copyBtnAriaLabel') copyBtnAriaLabel: string;
+  @Input('copyValue') copyValue: string = 'Missing \'copyValue\' @Input property';
+  @Input('tooltip') tooltip: string;
+  @Input('tooltipPlacement') tooltipPlacement: string = 'top';


Needs typedoc comments

dlabrecq · 2018-05-30T15:58:33Z

src/app/copy/copy-base.ts

+   *
+   * @param {boolean} copied True when copy action has been triggered
+   */
+  public set recentlyCopied(copied: boolean) {


Does this setter need to be public? I'm not sure it's used by the component itself, so perhaps it can be removed?

Similar to copyValueToClipboard, if I change this to private/protected I get build errors. It's possible I'm just missing something so I'll take another look.

It appears that only CopyBase sets the recentlyCopied property. Instead of invoking this:

this.recentlyCopied = true;

You can set the property like so:

this._recentlyCopied = true;

That way, you can get rid of the public setter.

Oh! Geez I feel silly now :) For some reason I was thinking the getters and setters were a package deal and I needed to use the setter. Was definitely overcomplicating this, good call!

dlabrecq · 2018-05-30T15:58:46Z

src/app/copy/copy-base.ts

+  /**
+   * Copy value to the user's system clipboard
+   */
+  copyValueToClipboard(accessibleName: string): void {


Can this method be marked protected?

So when I use protected, I get the aot error

ERROR in ng:///workingrepos/seanforyou23-patternfly-ng/src/app/copy/inline-copy/inline-copy.component.html (13,5): Property 'copyValueToClipboard' is protected and only accessible within class 'InlineCopyComponent' and its subclasses.

The default seemed to work ok so I was going with that. I'm looking into it more now though, feel free to make a suggestion if this is a familiar error

Instead of calling copyValueToClipboard directly within the HTML template, you could do something more like this:

copyValue(): void {
this.copyValueToClipboard(this.copyBtnAriaLabel);
}

Ah ok, I see what you mean. I was actually considering doing this before but with a new build I'm realizing that error was just a one-off. Thanks for explaining that alternative though.

Oh, I see in your other comment there is preference to make these protected, so yea I'll be taking this advice after all.

…service update package.json keywords to include angular 5 mention create block-level copy component create inline copy component create copy module create copy service add new components and service to demo navigation accessibility considerations add unit tests for new components and service copy service examples label is only required input prop support multiple copy components per view ensure semantic relation between label and copy button use button instead of repurposing icon element add tooltips for hover state examples that use callbacks to launch toast notifications tslint rules

namespace css classes rework html/css use better input property names

…components

ensure components and services can be consumed individually

tokenPanelOpen becomes expanded dedicated tooltip and aria label inputs token becomes copyValue eol on various files clean up examples export all modules align markup between inline and block versions basic, expanded, and multiple examples for block copy

these changes already proposed in a more appropriate PR

use better method of obtaining unique id define a shared config interface between inline and block copy add tests for tooltips add tooltipPlacement input property do not console errors from component only console error if copy service is in verbose mode tooltip module/config provided at component level DOCUMENT comes from @angular/common use better access modifiers use better examples ensure border surrounds copy buttons

add in-component feedback for copied action move commonalities to a shared base class tweak styles

remove redundant style properties add a border to better define bounds of expand button

remove BlockCopyConfig remove InlineCopyConfig remove CopyConfigBase add typedoc comments adjust namespace for unique id copyValueToClipboard is now protected

rename callback example to notification remove unnecessary public setter avoid method name conflict add typedoc comments

seanforyou23 · 2018-06-11T22:07:29Z

I did notice that when I have focus on a Copy button and hit Enter, the focus isn't maintained on the Copy button. I would expect that focus to remain on the action I selected unless there's a reason to shift focus

@dlabrecq any chance you know what gives on this? Been trying to figure this out but no luck so far. I see this working as expected with buttons on several other widgets, for example

https://rawgit.com/patternfly/patternfly-ng/master-dist/dist-demo/#/sparkline and
https://rawgit.com/patternfly/patternfly-ng/master-dist/dist-demo/#/emptystate

But in this component, focus falls into the abyss once you press the enter key. Setup looks the same for the event handlers.. I've tried all the obvious things like returning false, preventing default, etc.

I thought it may be related to the button's html being replaced but I get the same behavior even with static html.

I could programmatically re-place the focus on the button, but this seems out of the way and I'm hoping to find a more natural solution. Let me know if you have any ideas, I'll keep looking for a solution in the mean time. Thanks!

dlabrecq · 2018-06-12T14:38:49Z

I changed your code to use a checkbox instead of a button and saw the same results.

My guess is the HTML changes in a way that the original element (with focus) no longer exists? Looking at the code I see two icons.

<i
      class="fa fa-clipboard"
      aria-hidden="true"
      *ngIf="!recentlyCopied"></i>
    <i
      class="fa fa-check"
      aria-hidden="true"
      *ngIf="recentlyCopied"></i>

Instead of hiding and showing two different icons, perhaps you can just apply the different CSS selectors to the same icon? Probably a cleaner way to apply different styles, anyway. For example:

<i class="fa" aria-hidden="true" 
    [ngClass]="'{'fa-check': recentlyCopied, 'fa-clipboard': !recentlyCopied}"/>

dlabrecq · 2018-06-12T14:39:16Z

BTW, I noticed this CobyBase code emits a string. This should probably be a typed object with the name as a property. Otherwise, folks may need to parse a string to figure out which component generated the event.

this.copiedToClipboard.emit(${accessibleName} copied!);

dlabrecq · 2018-06-12T14:44:16Z

Looking at the CopyService and believe the issue is related to creating a new HTML element in order to perform the copy. Specifically, we're calling this:

textarea.select();

When I comment this out, focus remains. Unfortunately, that is required to copy; thus, we will have to reset focus manually.

dlabrecq · 2018-06-12T14:53:11Z

We should add a warning to ServiceCopy.copy() method to indicate that we are setting focus here. Alternatively, we can obtain the element that has current focus in order to restore it later. For example, we should be able to use document.activeElement or document.querySelector( ':focus' )

See: https://stackoverflow.com/questions/497094/how-do-i-find-out-which-dom-element-has-the-focus

Clean up view templates Emit typed object in response to copiedToClipboard event Programatically replace focus on trigger element upon copy

dlabrecq

Looking good. Just minor comments

dlabrecq · 2018-06-25T01:11:43Z

src/app/copy/copy-base.ts

+    if (result) {
+      this.copiedToClipboard.emit({
+        name: accessibleName,
+        msg: `${accessibleName} copied`


Can this event contain the copied value instead? I don't think the msg string will be very useful and it's not localizable.

Instead of providing the aria label here, I'm thinking that the copied value will be more appropriate. Unless we also have a component ID, name, etc. the aria label itself really doesn't identify which component generated the event. That is, considering the tooltip/aria label are optional, the event could be empty.

dlabrecq · 2018-06-25T01:12:36Z

src/app/copy/copy-base.ts

+   * The text node to be copied to the users clipboard
+   * @type {string}
+   */
+  @Input('copyValue') copyValue: string = 'Missing \'copyValue\' @Input property';


Can this be an empty string or left undefined? I'd prefer not to hard code strings as default properties. An application would have to parse this string in order to interpret its meaning. Testing the copy value for an empty string or undefined would be easier to determine if a value was obtained.

dlabrecq · 2018-06-25T01:44:36Z

src/app/copy/block-copy/block-copy.component.ts

+
+import { CopyService } from '../copy-service/copy.service';
+
+@Component({


Needs typedoc comments for the component itself. Can be very simple with basic import examples. See basic card for an example.

/** * Card component * * For customization, use the templates named headerTemplate and footerTemplate. * * Usage: * <br/><code>import { BasicCardModule } from 'patternfly-ng/card';</code> * * Or: * <br/><code>import { BasicCardModule } from 'patternfly-ng';</code> */

dlabrecq · 2018-06-25T01:47:03Z

src/app/copy/block-copy/example/block-copy-basic-example.component.ts

+    buttonLabel: 'Copy JSON',
+    tooltip: 'Example Swagger JSON Snippet',
+    expandBtnAriaLabel: 'Toggle Swagger JSON',
+    copyBtnAriaLabel: 'Copy Swagger JSON'


It appears that buttonLabel and copyBtnAriaLabel are used for the same HTML element. Can they be named similarly? For example, buttonLabel and buttonAriaLabel or copyBtnLabel and copyBtnAriaLabel.

Whatever you choose, please make inline-copy the same.

dlabrecq · 2018-06-25T01:53:58Z

src/app/copy/copy-service/copy.service.module.ts

+  ],
+  providers: [CopyService]
+})
+export class CopyServiceModule {}


Please remove this module. We're not declaring a component or directive, so a module isn't necessary. It's already exported via the index.ts file.

FYI, the notification service no longer has a module -- it's been deprecated.

dlabrecq · 2018-06-25T01:57:49Z

src/app/copy/copy-base.ts

+export interface CopiedMsg {
+  name: string;
+  msg: string;
+}


CopiedMsg should be exported via the index.js file.

I'd also like to rename this object as CopyEvent, located in its own file, with typedoc comments. This would be similar to other component events.

dlabrecq · 2018-06-25T02:23:57Z

src/app/copy/inline-copy/inline-copy.component.ts

+
+import { CopyService } from '../copy-service/copy.service';
+
+@Component({


Needs typedoc comments for the component itself. See basic card for an example.

dlabrecq · 2018-06-25T02:29:32Z

src/app/copy/copy-service/copy.service.ts

+
+    textarea.select();
+
+    if (!!triggerElement) {


Using !! is an obscure way to perform a type conversion. Please use one of the following approaches instead.

if (triggerElement !== undefined) {...} or
if (triggerElement) {...}

dlabrecq

Will address lingering code review comments in a new PR

seanforyou23 requested review from dlabrecq, jgiardino and sjcox-rh May 17, 2018 02:55

dlabrecq suggested changes May 17, 2018

View reviewed changes

seanforyou23 requested review from dongniwang and mcoker and removed request for sjcox-rh May 17, 2018 19:47

dlabrecq suggested changes May 21, 2018

View reviewed changes

seanforyou23 force-pushed the copy-clipboard branch from c88e0ac to 87a1e20 Compare May 23, 2018 21:37

mcoker reviewed May 24, 2018

View reviewed changes

dlabrecq suggested changes May 30, 2018

View reviewed changes

seanforyou23 added 13 commits June 4, 2018 16:58

[issue-335] namespace classes, use better prop names

53cdd76

namespace css classes rework html/css use better input property names

[incremental-commit] split inline copy example into multiple example …

21af7bc

…components

[issue-335] register all components and services as angular module

667dee4

ensure components and services can be consumed individually

[issue-335] address PR feedback

73c91c6

tokenPanelOpen becomes expanded dedicated tooltip and aria label inputs token becomes copyValue eol on various files clean up examples export all modules align markup between inline and block versions basic, expanded, and multiple examples for block copy

[cleanup] pkg json cleanup

fe46620

these changes already proposed in a more appropriate PR

[issue-335] incremental commit for addressing PR feedback

d0c6db5

add in-component feedback for copied action move commonalities to a shared base class tweak styles

[issue-335] cleanup

b5809a5

[issue-335] more cleanup

2ce3618

[issue-335] export modules in standard fashion

3f91fe8

[issue-335] styling cleanup

2f92669

remove redundant style properties add a border to better define bounds of expand button

[issue-335] add typedoc comments, remove config classes

84e89dd

remove BlockCopyConfig remove InlineCopyConfig remove CopyConfigBase add typedoc comments adjust namespace for unique id copyValueToClipboard is now protected

[issue-335] avoid method name conflict, remove unnecessary setter

bc94ece

rename callback example to notification remove unnecessary public setter avoid method name conflict add typedoc comments

seanforyou23 force-pushed the copy-clipboard branch from 2145edd to bc94ece Compare June 5, 2018 18:44

seanforyou23 added 2 commits June 13, 2018 10:20

[issue-335] fix focus after copy, use typed event objects

c31462f

Clean up view templates Emit typed object in response to copiedToClipboard event Programatically replace focus on trigger element upon copy

[issue-335] *use typed event objects

3ac57e4

dlabrecq force-pushed the master branch 2 times, most recently from 6d9f1f7 to 596afce Compare June 17, 2018 14:49

This was referenced Jun 20, 2018

"Add Account" modal part 1: Policy cloudigrade/frontigrade#11

Closed

11 account modal wizard cloudigrade/frontigrade#26

Merged

Copy (a value to clipboard) component patternfly/patternfly-react#423

Closed

dlabrecq suggested changes Jun 25, 2018

View reviewed changes

dlabrecq reviewed Jun 25, 2018

View reviewed changes

amysueg mentioned this pull request Jun 26, 2018

[UX] UX design for webhook connector syndesisio/syndesis#2740

Closed

dlabrecq approved these changes Jun 26, 2018

View reviewed changes

dlabrecq merged commit 50155d3 into patternfly:master Jun 26, 2018

sjcox-rh mentioned this pull request Jul 4, 2018

Copy to Clipboard [New Pattern] patternfly/patternfly-design#689

Closed

		@@ -0,0 +1,27 @@
		import { CopyConfigBase } from '../copy-config-base';


		import { CopyService } from '../copy-service/copy.service';

		@Component({

feat(clipboard-copy): add inline/block level copy component, copy service #351

feat(clipboard-copy): add inline/block level copy component, copy service #351

Uh oh!

Conversation

seanforyou23 commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlabrecq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgiardino commented May 18, 2018

Uh oh!

seanforyou23 commented May 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanforyou23 commented May 17, 2018 •

edited

Loading

dlabrecq May 17, 2018 •

edited

Loading

dlabrecq left a comment •

edited

Loading

seanforyou23 commented May 19, 2018 •

edited

Loading

dlabrecq May 21, 2018 •

edited

Loading

dlabrecq May 21, 2018 •

edited

Loading

dlabrecq May 21, 2018 •

edited

Loading

dlabrecq May 30, 2018 •

edited

Loading

dlabrecq Jun 5, 2018 •

edited

Loading