HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Social share widget for guubo.com

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
socialguuboforsharecomwidget

Problem

This is a social share widget for guubo.com. The live widget can be seen here (though you need to be signed in guubo.com, otherwise you will see a logged-out user interface). JS is not my first language, therefore some reviews are welcome.

```
// create iframe
var iframe = document.createElement('iframe');

iframe.setAttribute('style', 'border:none; overflow:hidden; width: 93px; height: 32px;');
iframe.setAttribute('scrolling', 'no');
iframe.setAttribute('frameborder', '0');
iframe.setAttribute('allowTransparency', 'true');

document.body.appendChild(iframe);

// access iframe object
iframe.doc = null;

if(iframe.contentDocument)
{
iframe.doc = iframe.contentDocument;
}
else if(iframe.contentWindow)
{
iframe.doc = iframe.contentWindow.document;
}
else if(iframe.document)
{
iframe.doc = iframe.document;
}

iframe.doc.open();
iframe.doc.close();

// load CSS to iframe window
var css_object = document.createElement('link');

css_object.rel = 'stylesheet';
css_object.type = 'text/css';

css_object.href = 'http://guubo.com/public/css/share.css?' + Math.random();

iframe.doc.head.appendChild(css_object);

var css_object = css_object.cloneNode('false');

css_object.href = 'http://guubo.com/public/css/reset.css?' + Math.random();

iframe.doc.head.appendChild(css_object);

// load CSS to parent window
var css_object = css_object.cloneNode('false');

css_object.href = 'http://guubo.com/public/css/share.css?' + Math.random();

iframe.doc.body.innerHTML = '';

function calculateCeneteredPosition(Xwidth, Yheight)
{
// First, determine how much the visitor has scrolled

var scrolledX, scrolledY;

if( self.pageYOffset ) {
scrolledX = self.pageXOffset;
scrolledY = self.pageYOffset;
} else if( document.documentElement && document.documentElement.scrollTop ) {
scrolledX = document.documentElement.scrollLeft;
scrolledY = document.documentElement.scrollTop;
} else if( document.body ) {
scrolled

Solution

Your code sets a lot of global variables, and you should always avoid them. The easiest way to do that is "to wrap your code in a closure and manually expose only those variables you need globally to the global scope."

Your script works a lot with DOM and DOM related objects. A lot of the goals you seem to be trying to reach for in that area could be achieved more expressively using the robust patterns found in a good library like jQuery, Prototype, Dojo, or Mootools. It is nearly always best to pick one of them and learn the API rather than trying to reinvent the wheel. Changes will be easier, and, likely, your code will be more cross-browser compatible. Don't use such a library for everything under the sun, but most of them cover a lot of common concerns.

With jQuery, for example, this line:

guubo_dialog.setAttribute('style', 'left: ' + position.left + 'px; top: ' + position.top + 'px;');


need not depend on an awkward string concatenation to set the style attributes. It could instead look much cleaner and be more adaptable:

$(guubo_dialog).css({
    'left': position.left + 'px',
    'top': position.top + 'px'
});


To keep your code style consistant, consider using JSLint or JSHint to tell you where you've gone wrong. guubo_dialog, for example, ought to be explicitly declared as a variable somewhere.

Code Snippets

guubo_dialog.setAttribute('style', 'left: ' + position.left + 'px; top: ' + position.top + 'px;');
$(guubo_dialog).css({
    'left': position.left + 'px',
    'top': position.top + 'px'
});

Context

StackExchange Code Review Q#2718, answer score: 3

Revisions (0)

No revisions yet.