-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Bento amp-copy
#35221
✨ Bento amp-copy
#35221
Conversation
extensions/amp-copy/1.0/component.js
Outdated
export function Copy({children, sourceId, text, ...rest}) { | ||
const [status, setStaus] = useState(null); | ||
let theme = useMemo(()=>{ | ||
console.log('theme'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these console logging lines for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will get this removed.
extensions/amp-copy/1.0/component.js
Outdated
const content = document.getElementById(sourceId); | ||
let text; | ||
if(content.value != undefined){ | ||
text = content.value.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume trimming is the desired behavior here so the user doesn't get spaces from the beginning/end of the input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
Hey @ampproject/wg-caching! These files were changed:
|
As this is a new component, has this been through design review approval yet? |
It hasn’t been through the design review process yet. Will now follow the process given here and try to have this reviewed during the next meet. In the meantime, please let me know if there’s anything that needs to be updated/added for this component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this contribution! I had a few questions and suggestions, and things are looking on the right track. In the mean time, it would be really helpful to have this go to design review with a concrete API (attributes, events, and actions, if any) so the wider community can discuss.
extensions/amp-copy/1.0/component.js
Outdated
text = content.textContent.trim(); | ||
} | ||
if (copyTextToClipboard(window, text)) { | ||
setStaus(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the first successful copy, subsequent successful copies won't cause an additional render. This is because when status === true
, setStatus(true) => noop
. Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion. It wasn't intended. We have just added the rendering support for the subsequent successful copies.
Remove unwanted if and else condition
Fix some typos Optimise the code
It is not valid AMP until the feature is ready for wider use. Files can be added back later when the AMP is ready
Also, prettify the code
Text passed in text attribute will get copied, if sorceId attribute is missing
It looks like CircleCI wants to see a fresh pull from |
It looks like the validator rules have been removed from the PR for now. Once this has been design reviewed, please ping |
Quick update: I have created |
@@ -0,0 +1,15 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the copyright header from all new files now :)
if (!ref.current) { | ||
return; | ||
} | ||
if (isCopyingToClipboardSupported(ref.current.ownerDocument)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would setting isCopySupported
be better suited with a callback ref? https://reactjs.org/docs/refs-and-the-dom.html#callback-refs
ref, | ||
() => | ||
/** @type {!CopyDef.CopyApi} */ ({ | ||
copyToClipboard: (selector = null, staticText = null) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be the behavior if both are provided? Should we emit an error message?
[ref, text] | ||
); | ||
|
||
const copyText = useCallback((textToCopy) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't copy
written to handle both the copyText and copySelector case?
ref={ref} | ||
className={objstr({ | ||
[classes.success]: status, | ||
[classes.failed]: status === false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[classes.failed]: status === false, | |
[classes.failed]: !status, |
* @param {{current: ?CopyDef.CopyApi}} ref | ||
* @return {PreactDef.Renderable} | ||
*/ | ||
export function CopyWithRef({children, sourceId, text, ...rest}, ref) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it may be worth aliasing this to textProp
so that you can then use the variable name text
in smaller scope locations instead of staticText
etc.
* Remove this comment! | ||
--> | ||
|
||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove this copyright header!
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
The goal of the PR is to support
copying to the clipboard
functionality.Facilitating clipboard copying is useful not only for AMP.dev but also for any site that has code samples.
Features
input
tags (includingtextarea
)div
,code
,pre
,p
tags)amp-copy
buttonIn Progress
Output/Demo
Fixes #4853