3
\$\begingroup\$

I'm trying to become better at javascript and have written a simple script that just creates some html from getting some details from some elements (e.g. the image alt text) and inserts it onto the page to create a caption. Hover over the top right of the image to see this caption.

I was able to successfully achieve what I wanted through my code. However, my javascript feels a bit 'wet', and I'd like to be able to improve this for brevity and clarity.

Can anyone recommend any improvements to my javascript, and make it either more concise or clear?

I know that jQuery can achieve this in fewer lines of code, but I'm less of a fan of jQuery and believe that ES6/ES7 etc can often write more concise code.

Thanks for any help here. Find a working demo below:

Codepen: https://codepen.io/anon/pen/PrwzBj

/* --- Wide Image Caption --- */
// Define Elements:
const wideImg = document.querySelector('.full-width-image-atf img'),
      captionText = wideImg.getAttribute('alt'),
      captionInnerHTML = '<span class="image-caption">' + captionText + '</span>',
      caption = document.createElement('div');
// Amend caption:
      caption.setAttribute('class', 'image-caption-wrap');
      caption.style.cssText = 'background: none;';
      caption.innerHTML = captionInnerHTML;
// Insert on page:
      wideImg.parentNode.appendChild(caption);
/* --- End of Wide Image Caption --- */
img {width: 100%}
.full-width-image-atf .content-main-image {
  position: relative;
  color: #fff;
}

.full-width-image-atf .image-caption-wrap {
  position: absolute;
  top: 0;
  right: 0;
  display: flex;
  flex-flow: row-reverse;
  align-items: center;
  padding: 15px 20px;
}
.full-width-image-atf .image-caption-wrap:before {
  content: '\f05a';
  font-family: 'Font Awesome 5 Pro';
  font-size: 2em;
}
.full-width-image-atf .image-caption {
  opacity: 0;
  padding-right: 10px;
}
.full-width-image-atf .image-caption-wrap:hover .image-caption {
  opacity: 1;
}
.full-width-image-atf .image-caption-wrap:hover {
  background: black !important;
  cursor: pointer;
}
.full-width-image-atf .content-main-image, .full-width-image-atf .content-main-image * { transition: 0.4s ease all }
<div class='full-width-image-atf'>
  <div class="content-main-image">
    <img src="https://static.independent.co.uk/s3fs-public/thumbnails/image/2018/12/31/10/lion-face.jpg?w968h681" alt="This is a test">
  </div>
</div>

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

General points

  • I think that multi line declarations are poor practice and can easily lead to global declarations if you have a miss placed ; and don't use the "use strict" directive.

  • As the DOM interface is rather verbose you can create some helper functions to reduce the amount of noise in your code and save yourself the trouble of having to repeat the same thing over and over..

  • Avoid inserting HTML directly via JavaScript, it is incredibly slow compared to using document.createElement, appendChild, etc...

  • Do not change the cursor to a pointer if there is nothing to click.

  • Rather than have an empty box (it is meaningless) put a question mark in there and create a box around it via CSS rule border..

Helper functions

Example of helper functions to do some common tasks. I never use jQuery and thus name the functions tag to $ and append to $$ to further reduce the code noise.

const query = (qStr, parent = document) => parent.querySelector(qStr);
const style = (element, style) => (Object.assign(element.style, style), element);
const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append = (parent, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), parent);

Note that each function returns an element so can be used as an argument for tag and append. There are many more helpers you can create to reduce the code verbosity.

or when not using jQuery

// $ creates tag, $$ appends elements
const query = (qStr, parent = document) => parent.querySelector(qStr);
const style = (element, style) => (Object.assign(element.style, style), element);
const $ = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const $$ = (parent, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), parent);

Example

With these function you can then simplify your code to

addImageCaption(query(".full-width-image-atf img"));  
function addImageCaption(img) {
    $$(img.parentNode,
        $$(style($("div", {className:"image-caption-wrap"}), {background: "none"}), 
            $("span", {className:"image-caption", textContent: img.alt})
        )
    );
}

Demo

Note that the CR snippet will not work when it contains the string $$

"use strict";
 
setTimeout(()=> addImageCaption(query(".full-width-image-atf img")), 0);

function addImageCaption(img) {
    append(img.parentNode,
        append(style($("div", {className:"image-caption-wrap"}), {background: "none"}), 
            $("span", {className:"image-caption", textContent: img.alt})
        )
    );
}


// DOM helpers 
const query = (qStr, parent = document) => parent.querySelector(qStr);
const style = (element, style) => (Object.assign(element.style, style), element);
const $ = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append=(par, ...sibs)=>sibs.reduce((p, sib)=>(p.appendChild(sib), p), par);
img {width: 100%}
.full-width-image-atf .content-main-image {
  position: relative;
  color: #fff;
}

.full-width-image-atf .image-caption-wrap {
  position: absolute;
  top: 0;
  right: 0;
  display: flex;
  flex-flow: row-reverse;
  align-items: center;
  padding: 15px 20px;
}
.full-width-image-atf .image-caption-wrap:before {
  content: '?';
  padding-left: 5px;
  padding-right: 5px;
  border: 1px solid white;
}
.full-width-image-atf .image-caption {
  opacity: 0;
  padding-right: 10px;
}
.full-width-image-atf .image-caption-wrap:hover .image-caption {
  opacity: 1;
}
.full-width-image-atf .image-caption-wrap:hover {
  background: black !important;
  cursor: help;
}
.full-width-image-atf .content-main-image, .full-width-image-atf .content-main-image * { transition: 0.4s ease all }
<div class='full-width-image-atf'>
  <div class="content-main-image">
    <img src="https://static.independent.co.uk/s3fs-public/thumbnails/image/2018/12/31/10/lion-face.jpg?w968h681" alt="This image is of a lion.">
  </div>
</div>

\$\endgroup\$
4
\$\begingroup\$

Not a comment on functionality, but here:

const wideImg = document.querySelector('.full-width-image-atf img'),
      captionText = wideImg.getAttribute('alt'),
      captionInnerHTML = '<span class="image-caption">' + captionText + '</span>',
      caption = document.createElement('div');

I don't think chaining declarations using , like you are is a good idea for a couple reasons:

  1. It isn't immediately clear that all the variables involved are being declared as const. Yes, you can tell if you check the indentation, or notice the commas at the end, but it isn't as clear as explicit consts all the way down.

  2. It makes refactoring more of a pain. Say in the future you want to break that up and maybe stick a log call in there or some other non-declaration. Now you need to manually replace the commas with semi-colons, and remember to add const to all the now separate declarations.

I'd suggest just writing it out fully from the start, and maybe space out the last one since it has a separate purpose:

const wideImg = document.querySelector('.full-width-image-atf img');
const captionText = wideImg.getAttribute('alt');
const captionInnerHTML = '<span class="image-caption">' + captionText + '</span>';

const caption = document.createElement('div');
\$\endgroup\$
3
\$\begingroup\$

I'd recommend looking into using a <template> tag, and possibly templates with slots unless the browser compatibility causes an issue. That way the template can remain in the markup, and all the Javascript logic might need to do is update the content that is dynamic (e.g. the caption text).


Instead of using setAttribute() to add a class name to element:

caption.setAttribute('class', 'image-caption-wrap');

There is a method: classList.add() that can be used instead:

caption.classList.add('image-caption-wrap');

I see the CSS has !important:

.full-width-image-atf .image-caption-wrap:hover {
  background: black !important;

It is wise to not depend on !important if possible, to avoid confusion. If you add the background: none style in the regular CSS then that should allow you to remove the !important.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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