2
\$\begingroup\$

I am have made a small copy-to-clipboard script.

I have made a method called copyToClipboard that does the following:

  1. create an input box;
  2. read the content of an HTML element;
  3. set the input box value to the content previously read;
  4. append the input box to a container;
  5. select and copy its value to the clipboard;
  6. remove the input box (no longer useful);

class ActivationCodeComponent{
  constructor() {
    this.copyButton = document.querySelector('#copy_button');
  }

  copyToClipboard () {
    const el = document.createElement('input');
    const codeBox = document.querySelector('#box');
    const activationCodeContainer = document.querySelector('#activation_code');
    el.value = activationCodeContainer.textContent;
    el.setAttribute('readonly', '');
    codeBox.appendChild(el);
    el.select();
    document.execCommand('copy');
    codeBox.removeChild(el);
  }
  
  handleClick() {
    this.copyButton.addEventListener("click", this.copyToClipboard);
  }

  init() {
    this.handleClick();
  }
}
const activationCodeComponent = new ActivationCodeComponent();
activationCodeComponent.init();
.container  {
   text-align: center;
}

#box  {
  border: 1px solid #ccc;
  width: 200px;
  margin: 20px auto;
  padding: 0 10px;
}

.code {
  letter-spacing: 2px;
  color: #069;
  font-size: 16px;
  font-weight: bold;
}
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/4.5.2/css/bootstrap.min.css" rel="stylesheet"/>
<div class="container">
  <div id="box">
    <div class="label text-uppercase small">Your activation code</div>
    <div class="code" id="activation_code">B12A7</div>
  </div>
  <button id="copy_button" class="btn btn-sm btn-success text-uppercase">Copy code</button>
 </div>

The problem

The validity of the script is too dependent on a set of HTML elements with the IDs I have used.

Could it be made more flexible?

\$\endgroup\$
3
\$\begingroup\$

Style

I can not fault the style and layout.

The need to call init which is an indirect call to handleClick seams redundant. Do it in the constructor.

I am puzzled as to why you set readonly to '' (el.setAttribute('readonly', '');) What has this to do with copying to the clipboard.

Names

Naming is very poor and also you use inconsistent naming conventions across JS (camelCase) and HTML (ids are all snake_case)

Names should provide the information needed to understand its behavior and content.

  • ActivationCodeComponent is a bit obscure giving no indication of what it does.

  • .handleClick Does not handle click (.copyToClipboard does that) .handleClick adds a click listener

  • el Too short and obscure. Maybe inputEl

  • codeBox What??? Maybe inputContainer

  • activationCodeContainer is not containing anything. Maybe copyTextSrcEl or textSourceElement

Basic rewrite

Rewrite using document.execCommand

Note that the rewrite does not return a use-able object thus no need to store it.

class ActivationCodeComponent{
    constructor() {
        function copyToClipboard () {
            inputEl.value = textSrcEl.textContent;
            inputContainer.appendChild(inputEl);
            inputEl.select();
            document.execCommand('copy');
            inputContainer.removeChild(inputEl);
        }  
        const inputEl = document.createElement('input');
        const inputContainer = document.querySelector('#box');
        const textSrcEl = document.querySelector('#activation_code');
        document.querySelector('#copy_button').addEventListener("click", copyToClipboard);
    }
}
new ActivationCodeComponent();

or

activationCodeComponent();
function activationCodeComponent() {
    function copyToClipboard () {
        inputEl.value = textSrcEl.textContent;
        inputContainer.appendChild(inputEl);
        inputEl.select();
        document.execCommand('copy');
        inputContainer.removeChild(inputEl);
    }  
    const inputEl = document.createElement('input');
    const inputContainer = document.querySelector('#box');
    const textSrcEl = document.querySelector('#activation_code');
    document.querySelector('#copy_button').addEventListener("click", copyToClipboard);
}

Problems

Deprecated

Document.execCommand is a depreciated function and should NOT be used.

Fix

You should be using Clipboard.writeText to add content to the clipboard. Note that the Code Review rewrite will not have clipboard permission and thus will fail.

Instantiatable and State

The best way to avoid bugs is to make impossible to incorrectly use the code.

Never allow conflicting or overlapping behaviors. As the code is, there are dozens of ways for it to be used in a manner that will create unpredictable behaviors.

  • There is nothing to prevent the object ActivationCodeComponent being created more than once.

  • The created object's state is exposed thus making all of its behavior un-trustable.

Fix

There is no need to create an instantiatable object.

  • Use a function's scope to contain the behavior and protect the state.
  • Ensure that the function can only be called once.

User feedback

There is no user feedback to indicate if the button click has done anything. Always give feedback to all actions no matter how trivial.

Fix

The rewrite adds a simple message element that displays the result of the copy action. The message is cleared after 3 seconds.

Question

You write

"The validity of the script is too dependent on a set of HTML elements with the IDs I have used."

"Could it be made more flexible?"

There are many ways to make the code more flexible.

  • Pass the code the elements required
  • Use Element's Element.dataset to mark objects

Example marks the copy button with the query needed to locate the text source.

 const copyBtn = document.querySelector("[data-copy-src]");
 const copySrcEl = document.querySelector(copyBtn.dataset.copyButton);
<div class="code" id="activationCodeEl">B12A7</div>
<button id="copyButton" data-copy-src="#copySrcEl">Copy code</button>

Rewrite

Example moves element queries out of the function. To use you must pass all the elements required for the function to work.

The rewrite performs the following

  • Marks the function to ensure that the code runs only once.
  • Checks all elements to make sure that they are instanceof Element.

const isElement = obj => obj instanceof Element;
const query = (qStr, root = document) => root.querySelector(qStr);
const queries = (qObj, root) => (
    Object.entries(qObj).forEach(([key, qStr]) => qObj[key] = query(qStr, root)),
    qObj
)
const copyElements = queries({
    button: "#copyButton", 
    textSrcEl: "#activationCodeEl", 
    messageEl: "#copyMessageEl",
});

copyToClipboard(copyElements);

function copyToClipboard({button, textSrcEl, messageEl}) {
    const okMessage = "Text copied to system copy buffer";
    const failMessage = "Text failed to copy";
    const okTypeClass = "copyOK", failTypeClass = "copyFail";
    var messageClearHdl;
    
    if (!isElement(button) || !isElement(textSrcEl) || !isElement(messageEl)) { return }
    if (copyToClipboard.active) { return }
    copyToClipboard = Object.freeze(
        Object.assign(copyToClipboard, { active: Symbol.for("activeCopy") })
    );

    function showMessage(text, typeClass) {
        clearTimeout(messageClearHdl);
        messageEl.textContent = text;
        messageEl.className = typeClass;
        messageClearHdl = setTimeout(() => {
                messageEl.textContent = "";
                messageEl.className = "hide";
            }, 3000);
    }
    button.addEventListener("click", () =>
        navigator.clipboard.writeText(textSrcEl.textContent).then(
            () => showMessage(okMessage, okTypeClass),
            () => showMessage(failMessage, failTypeClass)
        ));
}
.container  {
   text-align: center;
}

#box  {
  border: 1px solid #ccc;
  width: 200px;
  margin: 20px auto;
  padding: 0 10px;
}

.code {
  letter-spacing: 2px;
  color: #069;
  font-size: 16px;
  font-weight: bold;
}
.hide {
    display: none;
}
.copyOK {
    margin-top: -18px;
    font-family: arial;
    display: block;
}
.copyFail {
    margin-top: -18px;
    color: red;
    font-family: arial;
    display: block;
}
<div class="container">
  <div id="box">
    <div class="label text-uppercase small">Your activation code</div>
    <div class="code" id="activationCodeEl">B12A7</div>
  </div>
  <div id="copyMessageEl" class="hide"></div>
  <button id="copyButton" class="btn btn-sm btn-success text-uppercase">Copy code</button>
 </div>

Of course many will think that the context security is over the top. Apart from guaranteeing the code will behave as expected there is no need to include the context security.

const query = (qStr, root = document) => root.querySelector(qStr);
const queries = (qObj, root) => (
Object.entries(qObj).forEach(([key, qStr]) => qObj[key] = query(qStr, root)), qObj);

// example only so not making call
//copyToClipboard(queries({
//button: "#copyButton", textSrcEl: "#activationCodeEl", messageEl: "#copyMessageEl",
//}));

function copyToClipboard({button, textSrcEl, messageEl}) {
const okMessage = "Text copied to system copy buffer";
const failMessage = "Text failed to copy";
const okTypeClass = "copyOK", failTypeClass = "copyFail";
var messageClearHdl;

function showMessage(text, typeClass) {
    clearTimeout(messageClearHdl);
    messageEl.textContent = text;
    messageEl.className = typeClass;
    messageClearHdl = setTimeout(() => {
            messageEl.textContent = "";
            messageEl.className = "hide";
        }, 3000
    );
}
button.addEventListener("click", () => {
    navigator.clipboard.writeText(textSrcEl.textContent).then(
        () => showMessage(okMessage, okTypeClass),
        () => showMessage(failMessage, failTypeClass)
    );
});
}

\$\endgroup\$
1

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.