feat(copy-updates): align copy component impl with latest designs #444
Conversation
|
Deploy preview for patternfly-ng ready! Built with commit b640c9e |
src/app/copy/copy-base.ts
Outdated
| copyBtnTooltipPlacement: 'top', | ||
| copyBtnTooltipText: 'Copy to Clipboard', | ||
| copiedTooltipText: 'Copied' | ||
| }; |
There was a problem hiding this comment.
Please move the defaults to the corresponding inputs in this class. It looks like only "Copied" is used more than once?
There was a problem hiding this comment.
See latest updates, we're using 'Copy to Clipboard' and 'Copied' potentially twice so I left those in a defaults object, but if it still doesn't make sense - happy to move them out of here. Let me know what you think!
src/app/copy/copy-base.ts
Outdated
| setTimeout(() => { | ||
| this._recentlyCopied = false; | ||
| this.copyBtnTooltipText = this._copyDefaults.copyBtnTooltipText; |
There was a problem hiding this comment.
This is a little weird. The "Copied" tooltip appears to be set to the same default value twice; once on line 87, then on line 90 in the timeout? How is the tooltip restored to the original copyBtnTooltipText value provided by the developer?
this.copyBtnTooltipText = this._copyDefaults.copiedTooltipText;
Do we need to provide another tootltip property (e.g., tooltipCopiedText), so devs can localize their own "Copied" text?
There was a problem hiding this comment.
Good catch! Yes I think adding a dedicated property for the copied msg is the way to go, adding that to my latest changes. Also, there wasn't a way to restore to the original copyBtnTooltipText, so adding that as well.
| /** | ||
| * A width to set on the copy container | ||
| */ | ||
| @Input('width') width: string; |
There was a problem hiding this comment.
Is there a default width for the pfng-block-copy selector? Just wondering what happens when this property is not provided?
There was a problem hiding this comment.
We're basically not setting a default width so the inline copy remains inline flow content out of the box.
Block copy is encased within a div which is block level by default so it will occupy 100% of available width in the parent element.
Because the width is a shared property between inline and block, not setting a default value here seemed to make most sense. I could set width: 100% on .pfng-block-copy if you think this will become a tripping hazard for devs.
There was a problem hiding this comment.
That is fine. I was more curious if setting width was a requirement
improve tests fix tooltip flutter add tooltip for copy buttons use input element for copy value container remove redundant triggers clean up view templates use obj for storing common values in copyBase add project flag to tslint script to avoid "rule requires type information" warning use readonly input instead of span so copy value is scrollable use tooltipText input prop instead of tooltip to avoid namespace collision both focus and hover can launch a tooltip specify body for container attribute to fix tooltip styling issues avoid breaking changes by setting buttonAriaLabel to buttonLabel value when passed in update tests to use new property name
seanforyou23
force-pushed
the
copy-tooltip
branch
from
e0642b1
to
b640c9e
Compare
|
🎉 This PR is included in version 4.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR updates the copy clipboard components to use the latest designs and interactions specified in core.
One outstanding issue that I'm not sure how to resolve is that the tooltip text for the copy buttons updates to "Copied" for a few seconds after firing the copy action, however this value isn't updated in the DOM until you move your cursor away from the copy button and then back over the top of it again. I'm using property bound
[tooltip]syntax which I thought would handle this but doesn't seem to. Any idea how to solve this?make widths configurable
improve tests
add tooltip for copy buttons
specify tooltip container to fix tooltip flutter
use input element for copy value container
remove redundant triggers
clean up view templates
use obj for storing common values in copyBase
use readonly input instead of span so copy value is scrollable
use tooltipText input prop instead of tooltip to avoid namespace collision
specify body for container attribute to fix tooltip styling issues
avoid breaking changes by setting buttonAriaLabel to buttonLabel value when passed in
update tests to use new property name