m-danish-iqbal / section-scroll

jQuery plugin for automatically making scrollable sections navigation.
http://plugins.imdanishiqbal.com/section-scroll/
89 stars 27 forks source link

Scroll 1px on load #4

Closed sylvainbaronnet closed 8 years ago

sylvainbaronnet commented 8 years ago

Hello,

Can you explain why it scroll 1px on document ready ?

$('html, body').stop().animate({
  scrollTop: 1
}, 0);

At least there's no reason to stop() there

m-danish-iqbal commented 8 years ago

Plugin was unable to fetch first section when document loaded, so to overcome this issue i animated html by 1px that way it called the window scroll function and then it fetched the first section.

I just realised that i was setting fromTop variable in window scroll function properly but not in document ready, i have did the same for document ready now it can fetch first section without scrolling on load.

Thank you so much for your input.

Cheers

sylvainbaronnet commented 8 years ago

Glad to help

FYI, you could trigger a scroll event by doing $(window).scroll();

Also, your code is duplicate in $(document).ready and $(window).scroll. It's not a good thing (DRY!).

You can just trigger a scroll event on load just by doing this :

$(window).scroll(function () {

  // ...your code...

}).scroll(); // <- trigger scroll event

That way you could completely remove the $(document).ready() event code

Can you explain why you use a setTimeout() with no time passed as second argument ?

.find('a').filter('[href="#' + id + '"]') could be merged as .find('a[href="#' + id + '"]')

$('.' + $bullets_class).find("li") could be merged as $('.' + $bullets_class+' li')

$('.' + $bullets_class).find('span') could be merged...

Line 68 : item is already a jQuery object, you can use item.length directly (also I prefer item[0] syntax)

Another thing : it's a good pratice to store $( ) in var instead of calling it multiple times. $(this) should be store in .each() line 46

You store $(window) in $window but you don't use it line 115 and line 116 (you should)

line 59 : if (!(settings.titles)) not sure why you add () around settings.titles

m-danish-iqbal commented 8 years ago

Hi Sylvain

I'm traveling for a month and don't really have time to work on that so i have added you as a collaborator if you make any changes to the code you can push to 'update' branch.

Thanks

sylvainbaronnet commented 8 years ago

I made some major refactoring on the update branch. I let you review it and tell me if you have any question.

m-danish-iqbal commented 8 years ago

Sorry for late reply. Have you checked everything like options etc, I have checked the code it's working fine, great work. The only thing that is not working right now is $container.trigger('section-reached'); on page load we cannot detect the in view section because it's inside scroll function only, do you think we should fix that or should i push it anyway.

Thanks

sylvainbaronnet commented 8 years ago

Let me take a look

sylvainbaronnet commented 8 years ago

it's fixed. The scroll trigger on page load needed to be in a document ready event. It should be ok now.

m-danish-iqbal commented 8 years ago

Merged, awesome.

Cheers

sylvainbaronnet commented 8 years ago

Thanks for adding me as a contributor :)

m-danish-iqbal commented 8 years ago

Thank you for contributing mate :)