Improve my code?

[Learning]

Hi guys,

 

I have a simple caption for photos that can be hidden by a click of a button.
As my site has several of them, I decided to create a function to generate this effect across the different photos.

Using an empty object to hold the various items in each photo and click events to manipulate them.

But I felt it is a little scrappy using the object and am not sure if this is the best way to handle it.

There is no bugs or problems per se. I'm just wondering if anyone could help me suggest a better way to handle this functionality, or could this code be improved/shorten or anything? Just trying to learn to code better.
Thanks in advance guys!

 

// empty object to hold photos and their relevant parts
const eic = {};

function createExpandingButtons( pre, id ) {

	// store the various parts of the photo caption
	eic[ pre + 'Button' ]  = id.find( '.button' );
	eic[ pre + 'Arrow' ]   = id.find( '.button span' );
	eic[ pre + 'Caption' ] = id.find( '.caption' );
	eic[ pre + 'State' ]   = 1;

	eic[ pre + 'Button' ].on( 'click', { pre: pre }, function() {
		if ( eic[ pre + 'State' ] === 0 ) {
			eic[ pre + 'State' ] = 1;
			eic[ pre + 'Caption' ].css( 'position', 'relative' );
			TweenLite.to( eic[ pre + 'Arrow' ], 0.5, { rotation: -90, ease: Power4.easeOut } );
		} else {
			eic[ pre + 'State' ] = 0;
			eic[ pre + 'Caption' ].css( 'position', 'absolute' );
			TweenLite.to( eic[ pre + 'Arrow' ], 0.5, { rotation: 90, ease: Power4.easeOut } );
		}
	});
}


// example of creating one such photo caption
createExpandingButtons( '$indexHero', $( '#index-hero' ) );

 

[GreenSock]

It's a little tough to know how to structure things best without knowing more about all the functionality you need, but I'll take a stab at a slight rewrite...

// empty object to hold photos and their relevant parts
const eic = {};
function createExpandingButtons( pre, id ) {
    // store the various parts of the photo caption
    var obj = {
        button: id.find( '.button' ),
        arrow: id.find( '.button span' ),
        caption: id.find( '.caption' ),
        state: true
    };
    eic[ pre ] = obj;
    obj.button.on( 'click', { pre: pre }, function() {
        //toggle the state
        obj.button.state = !obj.button.state;
        if (obj.button.state) {
            obj.caption.css( 'position', 'absolute' );
            TweenLite.to( obj.arrow, 0.5, { rotation: 90, ease: Power4.easeOut } );
        } else {
            obj.caption.css( 'position', 'relative' );
            TweenLite.to( obj.arrow, 0.5, { rotation: -90, ease: Power4.easeOut } );
        }
    });
}

// example of creating one such photo caption
createExpandingButtons( '$indexHero', $( '#index-hero' ) );

 

It just seemed a bit cleaner to be able to reference things like eic[ pre ].button, plus we're using a local "obj" variable that'll be faster to lookup. 

 

There are definitely ways to use ternary statements to make your code shorter, but that can also make it a bit less readable so it's not necessarily "better". 

 

Happy tweening!

[Learning]

Hi Jack!

Thanks for the suggestions.
Yes, I do think that wrapping the sub items into an object to reference seem a lot cleaner.
Will definitely use that!
 

I notice you're using a toggle state too, that feels a lot better!
I've always been using an if/else statement and then changing the states inside them with a = 1 and = 0 and that does feel clunky.
Will steal this to use everywhere! =D

 

Thanks!!!