Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Added fixed header option #20

Merged
merged 1 commit into from
Jan 8, 2014
Merged

Conversation

a-v-l
Copy link
Contributor

@a-v-l a-v-l commented Jan 5, 2014

Websites using a fixed navigation bar can add the class "fixed-header" to the navigation element to offset the end of the scrolling animation at the right position - and not as default at the top of the page.

@cferdinandi
Copy link
Owner

@a-v-l - First, thanks so much for taking the time to submit a pull request. This is a cool idea.

Unfortunately, I don't think I'm going to merge it into the project, because it adds extra lines of code for functionality that will only be used by people with fixed headers (which won't be everyone). I'd rather see folks who have this need add the offset into the script themselves.

@a-v-l
Copy link
Contributor Author

a-v-l commented Jan 6, 2014

@cferdinandi No problem – you are the lead developer :-)

But I think your script will be much more versatile with this option. It was after I send the pull request when I found that I am not the only one with this usecase (#3).
My idea of adding an extra class for the fixed header provides a much more generic solution than just adding return distance - 20; because the elements height will be calculated by fixedHeader.offsetHeight - which takes care of different header styles in responsive websites.

If you are concerned about the extra lines, we could use a ternary operator to squeeze the code in two extra lines:

var fixedHeader = document.querySelector( '.fixed-header' );
var headerHeight = fixedHeader === null ? 0 : fixedHeader.offsetHeight;
return distance - headerHeight;

Running this code in googles closure compiler will result in 70 extra Bytes (1.425 vs 1.495) which I think is an acceptable amount. And this would save us "fixed header guys" to have to "hack" the core, enabling us to add your repository to the "third party" folder and not worry about updating…

Anyway – thank you for this cool script in the first place!

@cferdinandi
Copy link
Owner

@a-v-l - You've convinced me! One minor request: can we use .scroll-header instead of .fixed-header? Namespacing the class may seem a bit clunky, but it helps avoid possible conflicts with other classes.

@a-v-l
Copy link
Contributor Author

a-v-l commented Jan 7, 2014

Really happy to contribute!

See commit: No problem with using .scroll-header (although this adds an other Byte ;-) and I think it's a good point, keeping the namespace simple. Maybe if for some reason more classes are needed even .scroll could be changed to .scroll-link or .scroll-anchor

BTW: Do you want me to "clean up" before merging?

@cferdinandi
Copy link
Owner

If you wouldn't mind, that would be much appreciated. And I'll be sure to give you appropriate credit. Thanks again Arndt!

Websites using a fixed navigation bar can add the class "scroll-header" to the navigation element to offset the end of the scrolling animation at the right position - and not as default at the top of the page.
@a-v-l
Copy link
Contributor Author

a-v-l commented Jan 7, 2014

Here you go! Have a nice day over at Boston…

cferdinandi pushed a commit that referenced this pull request Jan 8, 2014
Add offset support for fixed headers
@cferdinandi cferdinandi merged commit 0043c34 into cferdinandi:master Jan 8, 2014
@cferdinandi
Copy link
Owner

Awesome, thanks Arndt!

cferdinandi pushed a commit that referenced this pull request Jan 8, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants