Feature/oui clipboard #174

varun257 · 2018-04-26T10:53:28Z

UU-832

feature: oui clipboard component

change oui tooltip from one time binding to one way binding so as to reuse tooltip
add new component clipboard

Demo: http://feature_oui_clipboard_build.ui-kit.ovh/#!/oui-angular/clipboard

AxelPeter · 2018-04-26T12:38:53Z

packages/oui-clipboard/README.md

+### Default
+
+```html:preview
+<oui-clipboard data-text="copy this text">


Unlike in our managers, we don't prefixing with data- in our templates

AxelPeter · 2018-04-26T12:42:28Z

packages/oui-clipboard/README.md

+### Clipboard contained in width of the parent
+
+```html:preview
+<div style="width:300px;">


No inline style in our preview.
Why not about using oui-field for this case ? It has sizing attribute :)

Or you can use helpers classes for our documentation.
See oui-doc-preview-only and oui-doc-preview-only-keep-children.

AxelPeter · 2018-04-26T12:45:19Z

packages/oui-clipboard/README.md

+    <oui-clipboard data-text="copy this text">
+    </oui-clipboard>
+</div>
+```


API table of the component attributes is missing

AxelPeter · 2018-04-26T12:46:14Z

packages/oui-clipboard/src/clipboard.component.js

+export default {
+    template,
+    controller,
+    controllerAs: "$clipctrl",


No need, we use the default one $ctrl for our template

The tooltip has $ctrl as controller because of which i lose scope of the parent controller. So had to rename this

Ok. In this case, I think we should rename the controller of the tooltip instead with $tooltipCtrl.

AxelPeter · 2018-04-26T12:46:53Z

packages/oui-clipboard/src/clipboard.controller.js

+        this.ouiClipboardConfiguration = ouiClipboardConfiguration;
+    }
+
+    $postLink () {


Should be $onInit
$postLink should only be use for DOM modifications :)

AxelPeter · 2018-04-26T13:01:42Z

packages/oui-clipboard/src/clipboard.html

+            data-ng-value="$clipctrl.text"
+            data-oui-tooltip="{{$clipctrl.tooltipText}}"
+            readonly>
+        <span class="oui-clipboard__icon" data-ng-if="$clipctrl.status === 'initial'"><i class="oui-icon oui-icon-copy-normal" aria-hidden="true"></i></span>


Since the icon must be clickable, this should be in a <button>

AxelPeter · 2018-04-26T13:03:49Z

packages/oui-clipboard/src/clipboard.html

@@ -0,0 +1,12 @@
+<div class="oui-clipboard">


This class could be added on the root component <oui-clipboard>, to avoid this DIV.
See our use of $postLink for that :)

AxelPeter · 2018-04-26T13:05:46Z

packages/oui-clipboard/src/clipboard.html

@@ -0,0 +1,12 @@
+<div class="oui-clipboard">
+    <div class="oui-clipboard__container">


Why not use oui-input-group oui-input-group_button ? We could even go with a new modifier if necessary (eg. oui-input-group_clipboard)

AxelPeter · 2018-04-26T13:08:15Z

packages/oui-clipboard/src/clipboard.provider.js

+        return this;
+    }
+
+    setStatus (status) {


Is this really necessary ? Do we have already a use of this method ?

AxelPeter · 2018-04-26T13:08:29Z

packages/oui-clipboard/src/clipboard.provider.js

+        return this;
+    }
+
+    setAction (copy) {


Same for this one too :)

using these functions in spec for test cases

varun257 · 2018-05-02T09:44:26Z

Hi @AxelPeter, Have done the changes as suggested. Please check if this is okay now

FredericEspiau · 2018-05-30T13:41:17Z

packages/oui-clipboard/src/clipboard.html

        contenteditable>
-    <button class="oui-button"><i class="oui-icon oui-icon-copy-normal" aria-hidden="true"></i></button>
+    <button class="oui-button">
+        <span class="oui-icon oui-icon-copy-normal"></span>


aria-hidden="true"

We should have a description on this button, like "Copy" or something like this :)

Yep I already told him face to face

I've added an aria-label with the tooltip text "Copy to clipboard"

jleveugle · 2018-05-31T08:07:18Z

packages/oui-clipboard/src/clipboard.html

        contenteditable>
-    <button class="oui-button"><i class="oui-icon oui-icon-copy-normal" aria-hidden="true"></i></button>
+    <button class="oui-button">
+        <span class="oui-icon oui-icon-copy-normal"></span>


We should have a description on this button, like "Copy" or something like this :)

FredericEspiau · 2018-06-01T15:06:53Z

packages/oui-clipboard/src/clipboard.provider.js

            copyToClipboardLabel: "Copy to Clipboard",
            copiedLabel: "Copied",
-            notSupported: "Not supported. Please copy the text manually"
+            notSupported: "Copy to Clipboard not supported. Please copy the text manually"


clipboard

Was like this in the guidelines :)

Made the change

migrate cui clipboard to oui-clipboard design changes as per cx specs tooltip added on hover of the element tooltip bindin chaned to one way binding from one time binding cui clipboard to oui

add default parameters remove unnecessary code add width based html in readme cui clipboard to oui

rewrite html for clipboard with input group rename tooltip and clipboard controller add api for tooltip and clipboard cui clipboard to oui

fix tests add test for id and name attributes

* feat: oui clipboard migrate cui clipboard to oui-clipboard design changes as per cx specs tooltip added on hover of the element tooltip bindin chaned to one way binding from one time binding cui clipboard to oui * feat: oui clipboard add default parameters remove unnecessary code add width based html in readme cui clipboard to oui * feat: oui clipboard rewrite html for clipboard with input group rename tooltip and clipboard controller add api for tooltip and clipboard cui clipboard to oui * feat(oui-clipboard): add attributes id, name fix tests add test for id and name attributes * refactor(oui-clipboard): replicate BEM changes * chore(oui-clipboard): code review * chore: code review * feat(oui-clipboard): use of clipboardjs library * feat(oui-clipboard): add fallback if no support * chore(oui-clipboard): update tooltip text * chore: code review

AxelPeter suggested changes Apr 26, 2018

View reviewed changes

neolitec added the feature label Apr 26, 2018

neolitec added the in progress label May 3, 2018

AxelPeter approved these changes May 14, 2018

View reviewed changes

AxelPeter requested review from neolitec, rfougier and whispyy May 14, 2018 11:29

AxelPeter added the cui migration label May 16, 2018

whispyy approved these changes May 17, 2018

View reviewed changes

whispyy force-pushed the feature/oui-clipboard branch from b68b1db to 0db6e48 Compare May 17, 2018 20:09

whispyy removed the in progress label May 17, 2018

AxelPeter added the quality check label May 18, 2018

varun257 changed the base branch from master to develop May 21, 2018 05:41

AxelPeter force-pushed the feature/oui-clipboard branch from eb0c18e to b872a1d Compare May 30, 2018 13:00

FredericEspiau previously requested changes May 30, 2018

View reviewed changes

jleveugle previously requested changes May 31, 2018

View reviewed changes

FredericEspiau reviewed Jun 1, 2018

View reviewed changes

AxelPeter force-pushed the feature/oui-clipboard branch 2 times, most recently from e210475 to c18a606 Compare June 4, 2018 11:44

AxelPeter added quality check ok ok for merge and removed quality check labels Jun 4, 2018

varun257 and others added 5 commits June 6, 2018 10:31

feat: oui clipboard

3b04ecd

migrate cui clipboard to oui-clipboard design changes as per cx specs tooltip added on hover of the element tooltip bindin chaned to one way binding from one time binding cui clipboard to oui

feat: oui clipboard

016f52b

add default parameters remove unnecessary code add width based html in readme cui clipboard to oui

feat: oui clipboard

1200e69

rewrite html for clipboard with input group rename tooltip and clipboard controller add api for tooltip and clipboard cui clipboard to oui

feat(oui-clipboard): add attributes id, name

88ee812

fix tests add test for id and name attributes

refactor(oui-clipboard): replicate BEM changes

70f4397

Axel Peter added 6 commits June 6, 2018 10:31

chore(oui-clipboard): code review

a70750e

chore: code review

4d54bc2

feat(oui-clipboard): use of clipboardjs library

d9918ff

feat(oui-clipboard): add fallback if no support

729bd82

chore(oui-clipboard): update tooltip text

bcb083a

chore: code review

eabf9d7

AxelPeter force-pushed the feature/oui-clipboard branch from c18a606 to eabf9d7 Compare June 6, 2018 08:34

AxelPeter merged commit 2546d9c into develop Jun 6, 2018

AxelPeter deleted the feature/oui-clipboard branch June 6, 2018 08:35

		@@ -0,0 +1,12 @@
		<div class="oui-clipboard">
		<div class="oui-clipboard__container">

Feature/oui clipboard #174

Feature/oui clipboard #174

Uh oh!

Conversation

varun257 commented Apr 26, 2018 • edited by AxelPeter Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

varun257 commented May 2, 2018

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

varun257 commented Apr 26, 2018 •

edited by AxelPeter

Loading