Hi I coded a simple app that allows user to generate border radius and then copy it to clipboard.
I already found out how big is the impact from your reviews guys. My code is slowly improving.
If someone could point out mistakes etc. in my code I would be grateful.
HTML:
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link rel="stylesheet" href="style/style.css">
<title>Border radius</title>
</head>
<body>
<main>
<div class="header">
<h2>CSS border radius</h2>
<p class="description">CSS property rounds the corners of an element's outer border edge.</p>
<a href="https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius" target="_blank">Learn more</a>
</div>
<div class='container'>
<div class="inputs">
<input type="text">
<input type="text">
<input type="text">
<input type="text">
<a href="#" class="copyBtn">
<img src="img/clipboard.svg" alt="clipboard">
</a>
</div>
<div class="shape"></div>
</div>
<button class="resetBtn">
<img src="img/refresh-outline.svg" alt="refresh">
</button>
</main>
<script src='script.js'></script>
</body>
</html>
JS
'use strict';
const radiusSettings = document.querySelectorAll('input');
const copySettings = document.querySelector('.copyBtn');
const resetBtn = document.querySelector('.resetBtn');
const box = document.querySelector('.shape');
let settings = [];
const clearSettings = () => {
settings = [];
};
const isUnit = (input) => {
const units = ['px', 'in', 'pc', 'pt', 'em', 'rem', '%'];
for (const unit of units) {
if (input.includes(unit)) return true;
}
return false;
};
const setRadius = (direction, value) => {
if (!isUnit(value)) value = value + 'px'; // set px if input is without unit
switch (direction) {
case 'TL':
box.style.borderTopLeftRadius = value;
settings.push(['border-top-left-radius: ', value]);
break;
case 'TR':
box.style.borderTopRightRadius = value;
settings.push(['border-top-right-radius: ', value]);
break;
case 'BR':
box.style.borderBottomRightRadius = value;
settings.push(['border-bottom-right-radius: ', value]);
break;
case 'BL':
box.style.borderBottomLeftRadius = value;
settings.push(['border-bottom-left-radius: ', value]);
break;
}
};
for (let i = 0; i < radiusSettings.length; i++) {
radiusSettings[i].addEventListener('blur', () => {
if (radiusSettings[i].value) {
switch (i) {
case 0:
setRadius('TL', radiusSettings[i].value);
break;
case 1:
setRadius('TR', radiusSettings[i].value);
break;
case 2:
setRadius('BR', radiusSettings[i].value);
break;
case 3:
setRadius('BL', radiusSettings[i].value);
break;
}
}
});
}
resetBtn.addEventListener('click', () => {
for (let i = 0; i < radiusSettings.length; i++) {
radiusSettings[i].value = '';
box.style.borderRadius = 0;
clearSettings();
}
});
copySettings.addEventListener('click', () => {
const el = document.createElement('textarea');
for (let i = 0; i < settings.length; i++) {
el.value += `${settings[i][0]}: ${settings[i][1]};\n`;
}
clearSettings();
document.body.appendChild(el);
el.select();
document.execCommand('copy');
document.body.removeChild(el);
});
CSS:
@import url("https://fonts.googleapis.com/css2?family=Roboto:wght@300;400;500&display=swap");
*,
*::after,
*::before {
padding: 0;
margin: 0;
-webkit-box-sizing: border-box;
box-sizing: border-box;
font-family: 'Roboto', sans-serif;
}
body {
height: 100vh;
display: -webkit-box;
display: -ms-flexbox;
display: flex;
-webkit-box-pack: center;
-ms-flex-pack: center;
justify-content: center;
-webkit-box-align: center;
-ms-flex-align: center;
align-items: center;
background: #343434;
}
main {
background-color: white;
width: 50%;
height: 30rem;
border-radius: 10px;
display: -webkit-box;
display: -ms-flexbox;
display: flex;
-webkit-box-orient: vertical;
-webkit-box-direction: normal;
-ms-flex-direction: column;
flex-direction: column;
position: relative;
}
.header {
background-color: #f1f1f1;
text-align: center;
padding: 2rem 2rem;
border-radius: 10px;
}
.header h2 {
font-weight: 500;
font-size: 1.2rem;
margin-bottom: 0.5rem;
}
.header .description {
font-size: 0.9rem;
font-weight: 300;
}
.header a {
color: #da4453;
text-decoration: none;
font-size: 0.8rem;
display: inline-block;
margin-top: 0.4rem;
font-weight: 500;
}
.container {
-webkit-box-flex: 1;
-ms-flex: 1;
flex: 1;
padding: 2rem 0;
}
.container .inputs {
display: -webkit-box;
display: -ms-flexbox;
display: flex;
-webkit-box-pack: center;
-ms-flex-pack: center;
justify-content: center;
-webkit-box-align: center;
-ms-flex-align: center;
align-items: center;
}
.container .inputs input {
padding: 0.5rem 0.5rem;
text-align: center;
width: 5rem;
}
.container .inputs input:not(:first-child) {
margin-left: 1rem;
}
.container .shape {
background: -webkit-gradient(linear, left top, right bottom, from(#bc4e9c), to(#f80759));
background: linear-gradient(to bottom right, #bc4e9c, #f80759);
width: 15rem;
height: 7rem;
margin: 0 auto;
position: relative;
top: 50%;
-webkit-transform: translateY(-50%);
transform: translateY(-50%);
}
.copyBtn {
margin-left: 1rem;
}
.copyBtn img {
width: 2rem;
}
.resetBtn {
position: absolute;
right: 1rem;
top: 1rem;
border: none;
cursor: pointer;
}
.resetBtn img {
width: 1.2rem;
}
/*# sourceMappingURL=style.css.map */
2 Answers 2
Make Copy to Clipboard more prominent Unless you tell the user that the button on the right copies the rule to the clipboard, they probably won't know that's what it does. Consider adding text around it (or at least a tooltip), and maybe a :hover rule to clearly show that it's clickable. Maybe also consider adding a tooltip to the refresh button - it doesn't refresh the page, but it resets the input fields.
Use precise variable names resetBtn is a good name since it clearly indicates what the variable contains. radiusSettings and copySettings, not so much:
One might well expect something named
settingsto be an object containing individual settings - but these are inputs and buttons, so better to call them that.copySettingscontains a single element, so it shouldn't be plural. Maybe usecopyToClipboardButton.Given that you're working with DOM elements, the
inputinconst isUnit = (input) => {sounds ambiguous - it's the input string, not an input element. Maybe call itinputValue.The
settingsvariable is an array of arrays of CSS rules. Maybe call itcssRulesinstead (or remove the variable entirely - see below)
Prefer array methods Regarding isUnit, when you want to go through an array and do something, usually the first thing to consider is whether you can use any of the many array methods to achieve it. Here, you can use Array.prototype.some - it's more semantic and concise than a for loop:
const units = ['px', 'in', 'pc', 'pt', 'em', 'rem', '%'];
const isUnit = inputValue => units.some(unit => inputValue.endsWith(unit));
(since these substrings always come at the end, better to use endsWith than includes)
settings array This is structured as an array of arrays, but the inner sub-arrays are only used for the creation of a string later:
el.value += `${settings[i][0]}: ${settings[i][1]};\n`;
How about putting just the string into the array instead, to avoid the unnecessary intermediate data structure, like this?
settings.push('border-top-left-radius: ' + value);
Or, even better:
Duplicate rules If someone changes a single input multiple times, they'll get duplicate rules, eg:
border-top-left-radius: : 1px;
border-top-left-radius: : 13px;
border-top-left-radius: : 135px;
Fix it by using an object indexed by rule name instead of an array for settings, so that prior rules with the same name get overwritten.
switch is usually unnecessarily verbose and WET so I'd recommend avoiding it if it happens to be possible to refactor by using an object or array instead of cases. Here, your setRadius and input listeners can be replaced entirely with:
const shortRuleNames = ['top-left', 'top-right', 'bottom-right', 'bottom-left'];
radiusInputs.forEach((input, i) => {
input.addEventListener('click', () => {
const ruleName = `border-${shortRuleNames[i]}-radius`;
cssRules[ruleName] = input.value;
});
});
Constructing the clipboard text As mentioned earlier, use array methods when possible. Your original approach:
for (let i = 0; i < settings.length; i++) {
el.value += `${settings[i][0]}: ${settings[i][1]};\n`;
}
can avoid the for loop by doing instead:
el.value = settings
.map(items => `${items[0]}: ${items[1]}`)
.join('\n');
Or, by using the cssRules object instead of the settings object:
el.value = Object.values(cssRules).join('\n');
In my review, I won't give any specific guidelines of what you should or shouldn't do. Instead, I will try to express my thought process for improving your code. I hope that will be useful for you.
I am reading the description of the problem you want to solve and running your application. Now I have a good understanding of what you want to achieve. I notice
the functionality of copying text to the clipboard. It's definitely independent of the main application logic so I immediately write the copyToClipboard function:
const copyToClipboard = (text) => {
const el = document.createElement('textarea');
el.style.position = 'absolute';
el.style.top = '0';
el.style.left = '0';
el.value = text;
document.body.appendChild(el);
el.select();
document.execCommand('copy');
document.body.removeChild(el);
};
Does the application have a state? Yes, because you use it to generate CSS text for copying it to clipboard and to update styles in UI to show the end result. How state should look like? The simplest representation is an object:
const initialState = {
topLeft: '0px',
topRight: '0px',
bottomRight: '0px',
bottomLeft: '0px',
}
Since we have a state we need to notify other parts of the code of its changes. Also, we need the ability to set and get a state. After a few iterations, I come up with the following class:
class State {
constructor(initial) {
this._state = initial;
this._subscribers = [];
}
set(partialState) {
this._state = { ...this._state, ...partialState };
this.notify();
}
notify() {
this._subscribers.forEach((cb) => cb(this._state));
}
get() {
return this._state;
}
onUpdate(cb) {
this._subscribers.push(cb);
}
}
The first thing I want to implement is updating box styles in UI reflected by updating the state. So I instantiate my state:
const state = new State(initialState);
I don't like that ordering of inputs in UI can affect business logic so I decide to use data attributes for inputs:
<input type="text" data-id="top-left">
<input type="text" data-id="top-right">
<input type="text" data-id="bottom-right">
<input type="text" data-id="bottom-left">
Now I need a way to map identifiers in UI to keys of my state object so I write idToKey function that conveniently uses object instead of switch to make it more concise:
const idToKey = (id) => ({
'top-left': 'topLeft',
'top-right': 'topRight',
'bottom-right': 'bottomRight',
'bottom-left': 'bottomLeft',
}[id]);
I want to save in the state only values with units so I write normalize function:
const normalize = (val) => {
if (val === '') return normalize('0');
const units = ['px', 'in', 'pc', 'pt', 'em', 'rem', '%'];
return units.some((u) => val.endsWith(u)) ? val : `${val}px`;
}
Now we have everything in place to add an event listener for the blur event for all inputs to update the state:
inputs.forEach((input) => {
input.addEventListener('blur', ({ target }) => {
state.set({ [idToKey(target.dataset.id)]: normalize(target.value) });
})
});
Next, we want to update UI so users will see changes. We subscribe to the state updates to reflect changes in the styles of the box. To clarify my intention I extract callback's body to separate function updateBox.
const updateBox = (box) => function ({ topLeft, topRight, bottomRight, bottomLeft }) {
box.style.borderTopLeftRadius = topLeft;
box.style.borderTopRightRadius = topRight;
box.style.borderBottomRightRadius = bottomRight;
box.style.borderBottomLeftRadius = bottomLeft;
};
state.onUpdate(updateBox(document.querySelector('.shape')));
Since the state is updated and its changes reflected in UI we can proceed to copy to the clipboard button. We need a function for converting state to CSS text.
const toCSS = ({ topLeft, topRight, bottomRight, bottomLeft }) => {
return [
`border-top-left-radius: ${topLeft}`,
`border-top-right-radius: ${topRight}`,
`border-bottom-right-radius: ${bottomRight}`,
`border-bottom-left-radius: ${bottomLeft}`,
].join('\n');
}
Finally we add event listener for the button:
document.querySelector('.copyBtn').addEventListener('click', () => {
copyToClipboard(toCSS(state.get()));
});
The only left thing is the reset button. To reset values we just need to update the state:
document.querySelector('.resetBtn').addEventListener('click', () => {
state.set(initialState);
});
After running the application I find that although the state is updated, users see old values in inputs. That's because UI inputs don't reflect changes in the state. To do this we need to get notified about state change and update all inputs: Input blurred -> Update state -> Update all inputs. In React it's called controlled input.
const inputs = document.querySelectorAll('input');
const syncInputs = (inputs) => function (data) {
inputs.forEach((input) => {
const key = idToKey(input.dataset.id);
input.value = data[key];
});
}
state.onUpdate(syncInputs(inputs));
That's it. Here is the complete code:
class State {
constructor(initial) {
this._state = initial;
this._subscribers = [];
}
set(partialState) {
this._state = { ...this._state, ...partialState };
this.notify();
}
notify() {
this._subscribers.forEach((cb) => cb(this._state));
}
get() {
return this._state;
}
onUpdate(cb) {
this._subscribers.push(cb);
}
}
const idToKey = (id) => ({
'top-left': 'topLeft',
'top-right': 'topRight',
'bottom-right': 'bottomRight',
'bottom-left': 'bottomLeft',
}[id]);
const initialState = {
topLeft: '0px',
topRight: '0px',
bottomRight: '0px',
bottomLeft: '0px',
}
const state = new State(initialState);
const updateBox = (box) => function ({ topLeft, topRight, bottomRight, bottomLeft }) {
box.style.borderTopLeftRadius = topLeft;
box.style.borderTopRightRadius = topRight;
box.style.borderBottomRightRadius = bottomRight;
box.style.borderBottomLeftRadius = bottomLeft;
};
state.onUpdate(updateBox(document.querySelector('.shape')));
const inputs = document.querySelectorAll('input');
const syncInputs = (inputs) => function (data) {
inputs.forEach((input) => {
const key = idToKey(input.dataset.id);
input.value = data[key];
});
}
state.onUpdate(syncInputs(inputs));
state.notify();
const normalize = (val) => {
if (val === '') return normalize('0');
const units = ['px', 'in', 'pc', 'pt', 'em', 'rem', '%'];
return units.some((u) => val.endsWith(u)) ? val : `${val}px`;
}
inputs.forEach((input) => {
input.addEventListener('blur', ({ target }) => {
const newState = { [idToKey(target.dataset.id)]: normalize(target.value) };
state.set(newState);
})
});
const toCSS = ({ topLeft, topRight, bottomRight, bottomLeft }) => {
return [
`border-top-left-radius: ${topLeft}`,
`border-top-right-radius: ${topRight}`,
`border-bottom-right-radius: ${bottomRight}`,
`border-bottom-left-radius: ${bottomLeft}`,
].join('\n');
}
const copyToClipboard = (text) => {
const el = document.createElement('textarea');
el.style.position = 'absolute';
el.style.top = '0';
el.style.left = '0';
el.value = text;
document.body.appendChild(el);
el.select();
document.execCommand('copy');
document.body.removeChild(el);
};
document.querySelector('.copyBtn').addEventListener('click', () => {
copyToClipboard(toCSS(state.get()));
});
document.querySelector('.resetBtn').addEventListener('click', () => {
state.set(initialState);
});