Suggestions for plugin JS

Resolved mediumdeviation
(@mediumdeviation)

11 years, 2 months ago
I’ve read through the JS the plugin uses and have a few suggestions.

jQuery('#sb_instagram.sbi').each(function(){

This line here is bad, mainly because having multiple elements on the page with the same id is not valid HTML, and what you’re doing here (looping through multiple elements with the same id) isn’t technically speaking valid. Changing this to a class would make it valid, and enable users to use more than one of the widget on the page without generating invalid HTML.

parseInt is used with only one parameter multiple times, some on user input values. If parseInt receives a string starting with 0, it assumes the string is an octet and parses it as if it is base 8 unless 10 is passed as the second parameter, base. This is especially bad here because you’re using parseInt to parse userId, and if the user accidentally adds in a 0 in front of his ID he would find himself potentially loading someone else’s Instagram feed.

jQuery(a).attr("data-date") should be with jQuery(a).data('date') – jQuery made some major changes to the way attr works in 1.6, so that the now most of the time when you use attr(), you really should be using prop(), or in this case, since you’re using data-* attributes, data().

$self.find('.sb_instagram_header .sbi_header_img_hover').fadeIn(200) – this could be changed to use pure CSS using transition instead of JavaScript, which is more complex and less performant.
```
.sbi_photo {
    transition: opacity 0.3s;
} 

.sbi_photo:hover {
    opacity: 0.85;
}
```
return (Math.round(Math.random())-0.5); – using Math.random() for shuffling isn’t a very good idea. The amount of actual shuffling that happens is strongly dependent on the browser’s implementation of sort, and in most browsers would not produce an unbiased distribution. It’s not terribly important here, but changing it to the correct code is fairly trivial – see: http://stackoverflow.com/q/962802/313758

https://ww.wp.xz.cn/plugins/instagram-feed/

Viewing 4 replies - 1 through 4 (of 4 total)

Thread Starter mediumdeviation
(@mediumdeviation)

11 years, 2 months ago
This is the code I’m adding to the custom JavaScript under display options to help with the loading problem. I think it’s entirely find to include this in the plugin itself.
```
$('.sbi_item.sbi_new img').each(function(){
	$(this).fadeTo(0, 0).on('load', function(){
		$(this).fadeTo(200, 1);
	});
});
```
Plugin Author smashballoon
(@smashballoon)

11 years, 2 months ago

Hey mediumdeviation,

Thanks so much for the feedback and suggestions! This is really great and I’ll implement most of these into the next updates to the plugin. Some of the development decisions where made to accommodate legacy browsers like IE8 unfortunately, but it looks like most of these suggestions won’t adversely affect that anyway.

I’ve considered removing the ID from the feed before, however it’s difficult due to the fact that much of the JavaScript and existing CSS depends on it, and from what I’ve read using an ID in JS is much more performant than a class. I could potentially iterate through IDs, however a number of existing users use custom CSS or JavaScript which depend on the #sb_instagram ID, and so unfortunately changing it would cause problems for those people. As much as I agree with your suggestion, it seems like at this point changing it would cause more issues than the relatively minor benefit of passing HTML validation.

Thanks again for all of the quality feedback and code improvements, it’s hugely appreciated!

John

Thread Starter mediumdeviation
(@mediumdeviation)

11 years, 1 month ago

RE: HTML, I think it would be best to have the id/class attributes, I think it would be best to have a toggle somewhere for it, and have it turned off (ie. use class) for new installation, while keeping it on (ie. use id, so it doesn’t break anything) for upgrading users.

This way you get to keep backwards compatibility while making sure that the plugin’s HTML stays valid moving forward.

Plugin Author smashballoon
(@smashballoon)

11 years, 1 month ago

That’s a good suggestion. I’ve made a note of this idea and will give it some more thought to see whether I can get it implemented into a future update.

Thanks again!

John

Viewing 4 replies - 1 through 4 (of 4 total)

The topic ‘Suggestions for plugin JS’ is closed to new replies.

Tags