Skip to content

Instantly share code, notes, and snippets.

@scottrippey
Created August 24, 2012 17:43
Show Gist options
  • Save scottrippey/3453356 to your computer and use it in GitHub Desktop.
Save scottrippey/3453356 to your computer and use it in GitHub Desktop.
Code Review of FixedHeader.js
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));
}
});
@scottrippey
Copy link
Author

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.

@scottrippey
Copy link
Author

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".

@scottrippey
Copy link
Author

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