Created
August 24, 2012 17:43
-
-
Save scottrippey/3453356 to your computer and use it in GitHub Desktop.
Code Review of FixedHeader.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Uverse.widgets.FixedHeader = new Class({ | |
Implements: [ Options ] | |
, el: null | |
, sectionsNavigationSelector: '.sections-navigation' | |
, _fixedZIndex: 15 | |
, _sectionsNavigation: null | |
, _regularBodyPaddingTop: null | |
, _regularMastheadMarginTop: null | |
, _onScrollHandler: function() { | |
var bodyPaddingTop; | |
// If the scroll value is greater than the sectionsNavigation offset top | |
// we should fix it. The reason to use > instead of === is that browsers | |
// won't trigger this for every scrolled pixel. | |
if(window.scrollY > this._sectionsNavigation.offsetTop) { | |
// We change the style of `el`: | |
// - `width: 100%` because we want this to occupy all the screen wide. | |
// - `position: fixed` to fix it, duh. | |
// - `margin-top' is the weird one, we want to push the header top | |
// but we want to add padding to avoid a nasty "jump" | |
// caused by missing floating (fixing). | |
// - `z-index: 10` because we need to float over every element. | |
this.el.setStyles({ | |
'width': '100%', | |
'position': 'fixed', | |
'margin-top': -2 * this._sectionsNavigation.offsetTop, | |
'z-index': this._fixedZIndex | |
}); | |
// Set the body top padding to the offset of the navigation in order | |
// to avoid a "jump" caused by floating the masthead. | |
bodyPaddingTop = this._sectionsNavigation.offsetTop; | |
} else { | |
// If we are in a static safe zone, we can go ahead and return everything | |
// to the normal state. | |
this.el.setStyles({ | |
'position': 'static', | |
'margin-top': this._regularMastheadMarginTop | |
}); | |
bodyPaddingTop = this._regularBodyPaddingTop; | |
} | |
// @note: Should we move this inside the if statements? | |
document.body.setStyle('padding-top', bodyPaddingTop); | |
} | |
, initialize: function(el, options) { | |
this.el = $(el); | |
this.setOptions(options); | |
// Select the sections navigation… | |
this._sectionsNavigation = el.getElement(this.sectionsNavigationSelector), | |
// …and save the regular paddings and margins that will change on scroll. | |
this._regularBodyPaddingTop = document.body.getStyle('padding-top'); | |
this._regularMastheadMarginTop = el.getStyle('margin-top'); | |
document.addEvent('scroll', this._onScrollHandler.bind(this)); | |
} | |
}); |
Overall design:
Instead of adjusting the document's "padding-top", I'd suggest the following:
Set the nav bar's position to "absolute". Do not set a "top" or "left".
Insert a spacer (height: 50px) behind the nav bar to affect layout.
When the page scrolls, change the nav bar's "absolute" to "fixed". No padding or margin needs to change, and the browser might not need to do a "reflow".
Lines 22, 34: this.el.setStyles(...
We should not have so many styles in our JS.
Instead, we should use this.el.addClass("detached")
and removeClass
.
In the CSS, .detached
should define z-index
etc...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Line 22: this.el.setStyles(....
Code within a "scroll" event handler should be as minimal as possible to avoid interference, so:
We shouldn't repeatedly call "setStyles" . We should manage an internal boolean (ie. "isHeaderDetached"), and only change the element when that value changes.