3
\$\begingroup\$

I've created a basic site with thumbnail images which when clicked open a modal type window to show a larger image with info related specifically to the image.

It works as I would like. However, I'm trying to develop my limited JS skills and looking at the code, I think there is most probably an easier and cleaner (and more professional way) to create what I have but with far less code. I'm also sure there are a few bad practices in there which I should be aware of.

I'm wondering if someone could have a look through and make some suggestions/ corrections. My main concern is that I have an array of images with related details which I use a forEach() function to create the thumbnails. I then use a for loop to add the event listener which is probably not the best technique. I'm sure the two could be combined somehow. Also, how can I transfer information (related to the specific image i.e. description) from the array to the forEach() function and then to the for loop? My innerText method was a quick solution to get the job done.

I would prefer to keep an array to store the info rather than store it in the elements (directly in the body section) as this will be useful later on.

I considered using an object constructor to create each image but I wasn't sure how to do this and create individual images or if this was possible.

var photos = [
{ name: "red", src: "https://upload.wikimedia.org/wikipedia/commons/thumb/2/25/Red.svg/2000px-Red.svg.png", description: "red square" },
{ name: "blue" , src: "https://upload.wikimedia.org/wikipedia/commons/thumb/f/fd/000080_Navy_Blue_Square.svg/600px-000080_Navy_Blue_Square.svg.png", description: "blue square" },
{ name: "yellow" , src: "https://upload.wikimedia.org/wikipedia/commons/f/f2/Yellow_Square.JPG", description: "yellow square" }
];  

var body;
var modal_background;

body = document.getElementsByTagName('body')[0];
modal_background = document.createElement('div');
body.appendChild(modal_background);

photos.forEach(function(photo)  
{
var thumbs_container = document.getElementById('thumbs_container');
var thumb_container = document.createElement('div');
thumb_container.className = 'thumb_container';
var thumb_image = document.createElement('img');
thumb_image.className = 'thumb';
thumb_image.src = photo.src;
thumbs_container.appendChild(thumb_container);
thumb_container.appendChild(thumb_image);
var photo_info = document.createElement('div');
photo_info.innerHTML = '<p id="photo_date">' + photo.description + '</p>';
thumb_image.appendChild(photo_info);
});

var photos = document.getElementsByClassName('thumb');
var i;
for (i = 0; i < photos.length; i++)
{
photos[i].addEventListener('click', function(e){
    modal_background.className = 'background';

    var container = document.createElement('div');
    container.id = 'container';
    var img_container = document.createElement('div');
    img_container.id = 'image_container';
    var image = document.createElement('img');
    image.src = e.target.src;
    image.className = 'large';

    var photo_info_large = document.createElement('div');
    var date_from_thumb = e.target.firstChild.innerText;
    photo_info_large.innerHTML = '<p>' + date_from_thumb + '</p>';
    photo_info_large.id = 'photo_info';
    photo_info_large.style.display = 'none';

    var info_icon = document.createElement('img');
    info_icon.src = 'https://upload.wikimedia.org/wikipedia/commons/thumb/e/e4/Infobox_info_icon.svg/1024px-Infobox_info_icon.svg.png';
    info_icon.id = 'info_icon';
    info_icon.style.opacity = '0.3';
    info_icon.title = 'Information';

    var close_icon = document.createElement('img');
    close_icon.src = 'https://upload.wikimedia.org/wikipedia/commons/thumb/8/8d/VisualEditor_-_Icon_-_Close.svg/2000px-VisualEditor_-_Icon_-_Close.svg.png';
    close_icon.id = 'close_icon';
    close_icon.style.opacity = '0.3';

    body.appendChild(container);
    container.appendChild(img_container);
    img_container.appendChild(image);
    img_container.appendChild(info_icon);
    img_container.appendChild(close_icon);
    img_container.appendChild(photo_info_large);


        info_icon.addEventListener('mouseover',function()
        {
            info_icon.style.opacity = '0.8';
        }, false);

        close_icon.addEventListener('mouseover',function()
        {
            close_icon.style.opacity = '0.8';
        }, false);

        info_icon.addEventListener('mouseout',function()
        {
            info_icon.style.opacity = '0.3';
        }, false);

        close_icon.addEventListener('mouseout',function()
        {
            close_icon.style.opacity = '0.3';
        }, false);

        close_icon.addEventListener('click', function()
        {
        body.removeChild(container);
        modal_background.className = '';
        }, false);

        info_icon.addEventListener('click', function()
        {
        if (photo_info_large.style.display == 'block')
            {
                photo_info_large.style.display = 'none';
            }
        else {
                photo_info_large.style.display = 'block';
            }
        }, false);
}, false);
};
#thumbs_container {
	width: 940px;
	margin: 0 auto;
}

.thumb_container {
	display: inline-block;
	margin: 0 10px 0 10px;
}

.thumb {
    display: inline-block;
    width: 100px;
    background: black;
    z-index:1;
}

.background {
    top: 0;
    left: 0;
    position: absolute;
    width: 100%;
    height: 100%;
    background: black;
    opacity: 0.9;
    z-index: 100;
}

#container {
	position: absolute;
	top: 100px;
	left: 0;
	width: 100%;
	text-align: center;
	z-index: 300;
}

#image_container {
	position: relative;
    width: 250px;
	margin: 0 auto 0 auto;
    z-index: 400;
}

img {
    width:100%;
    }

img.large {
    width:100%;
	z-index: 500;
    }
	
#photo_info {
	position: absolute;
	left: 0;
	top: 0;
	width:100%;
	height: 100%;
	background: black;
	opacity: 0.7;
	color: white;
	z-index: 600;
}
	
#info_icon {
	position: absolute;
	width: 30px;
	top: 10px;
	left: 10px;
	z-index: 700;
}

#close_icon {
	position: absolute;
	width: 30px;
	top: 10px;
	right: 10px;
	z-index: 705;
}

	
#footer {
	left: 0;
	position:fixed;
	width:100%;
	height:20px;
	bottom:0;
	background:#383838 ;
	}
	
#footer_link {	
		list-style-type: none;
		padding:0;
		margin:0;
		color:white;
		text-align: center;
		font-size:15px;
		font-weight:300;
		}
<div id = 'thumbs_container'></div>

\$\endgroup\$
  • \$\begingroup\$ In the meantime, some quick points: the normal way to listen on many objects is to set a listener on a parent object, let it bubble up, then check the identity of the event target; you don't need to remember info between two loops if you only have one loop; you can create new properties on HTMLElement objects; you can set new attributes with names starting with data- to contain textual info. \$\endgroup\$ – Amadan Nov 16 '15 at 0:58
  • 1
    \$\begingroup\$ Have rolled back your changes to previous version. Please do not change the code after answers has been added, as that invalidates the answers. If you are satisified with the answers, and have accepted answer and want review on a new version, please post as a new question. Do not change the code in original question. \$\endgroup\$ – holroy Nov 17 '15 at 2:03
1
\$\begingroup\$

This is not a bad first attempt.

Stylistically:

  • Variable names are written in camelCase not snake_case
  • Opening { characters should occur on the same line as the if or loop:

    if (condition) {
        // ...
    else if (condition) {
        // ...
    } else {
        // ...
    }
    

Architecturally:

  • You want to avoid global variables like the Plague. There are two ways of accomplishing this:

    1. Use an Immediately Invoked Function Expression (IIFE)

      (function() {
          // Your code goes here
      })();
      

      This creates a function closure that prevents any variables declared using the var keyword from leaking out into the global context (i.e. the window object).

    2. Create a "class" in JavaScript and go the object oriented route. The advantage here is you can make your code modular and reusable.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you. Have noted your advice on the variable naming and curly braces. Have amended the code to remove the global variables essentially moving the body and modal variables to the event listener. \$\endgroup\$ – user4612360 Nov 16 '15 at 23:52
  • \$\begingroup\$ Regarding your last comment to create a class. I'm assuming you mean something along the lines of function Square(color, size){ this.color = color; this.size = size; } var redSquare = new Square('red', 'medium'); var blueSquare = new Square('blue', 'large'); // which creates instances but then how do I loop through/ forEach on these to create the individual elements when they are not in an array? Or would this be done via a method inside the class/ instance object? \$\endgroup\$ – user4612360 Nov 17 '15 at 0:06
0
\$\begingroup\$

My points:

  • Data stay in JS is good, you'd either get it from Ajax, or build it from the HTML.
  • Building the DOM by hand is very verbose, for simple case I will usually use tweet-sized template engine, and put the result in a DOM Fragment, for example:

    function render(s,d){
      for(var p in d) {
        s=s.replace(new RegExp('{'+p+'}','g'), d[p]);
      }
      return s;
    }
    
    var markup = photos.map(render.bind(null, '<img alt="{description}" src="{src}"/>')).join('');
    
  • Manipulating style in JS is considered unsafe, since browser implement it inconsistently. Usually I would manipulating CSS classes and have styling there.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I wouldn't say manipulating the style object is "unsafe." This suggests there are security concerns with doing that. I wouldn't even say it's implemented all the inconsistently -- at least no less consistent than browser support for CSS in general. I would say it is not a "best practice" and that manipulating CSS classes provides code that is easier to maintain. \$\endgroup\$ – Greg Burghardt Nov 16 '15 at 14:54
  • \$\begingroup\$ Thanks for the reply. Trying to make sense of your shortened code but proving a challenge at this stage. Noted advice on styling in JS. Thanks \$\endgroup\$ – user4612360 Nov 16 '15 at 23:50
  • \$\begingroup\$ That's true, there's no security concern here. But it is inconsistent, for example: vendor-prefixed properties. \$\endgroup\$ – tungd Nov 16 '15 at 23:57

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.