Improved scrolling behaviour for accordion without animation

[herrschuessler]

Hey, I did some improvements for the scrollToAnchor behaviour, which didn't work as expected / at all for accordions without animation.

I also included the option to automaticaly calculate body offset and add this to the scroll offset. This is usefull when a fixed header navigation is present.

I also did some cleanup of the CSS classes and loosened the prerequisites for the tab HTML structure.

[ajmueller]

I recently implemented something similar to this in a project I'm working on. Rather than have _scrollOffset be a Boolean, I made it an element selected with jQuery and call it scrollToAccordionOffsetElement. Defaulting to false, you can instead pass in a jQuery object (e.g. $('.nav-primary')) whose height is then calculated as the additional offset. This fixed our use-case where a fixed element at the top of the page (e.g. a primary navigation container) covered the accordion title when it was scrolled to. @jellekralt I'd be happy to mock up a quick PR with this change.

[herrschuessler]

@ajmueller That sounds like a cleaner solution than my code. It would still be important to measure the height of the offset element each time a jump to a target anchor occurs, since the height could change depending on scroll position.

[ajmueller]

@herrschuessler yep! I'm doing that as part of the _openTab function. Here's what I changed it to:

/**
 * This function opens a tab
 * @param {Event} e - The event that triggers the tab opening
 * @param {Object} oTab - The tab object that should be opened
 * @param {Boolean} closeCurrent - Defines if the current tab should be closed
 * @param {Boolean} stopRotation - Defines if the tab rotation loop should be stopped
 */
ResponsiveTabs.prototype._openTab = function(e, oTab, closeCurrent, stopRotation) {
    var _this = this,
        scrollOffset,
        activeTab,
        hashIsPresent = window.location.hash !== '';

    // Check if the current tab has to be closed
    if(closeCurrent) {
        this._closeTab(e, this._getCurrentTab());
    }

    // Check if the rotation has to be stopped when activated
    if(stopRotation && this.rotateInterval > 0) {
        this.stopRotation();
    }

    // Set this tab to active
    oTab.active = true;
    // Set active classes to the tab button and accordion tab button
    oTab.tab.removeClass(_this.options.classes.stateDefault).addClass(_this.options.classes.stateActive);
    oTab.accordionTab.removeClass(_this.options.classes.stateDefault).addClass(_this.options.classes.stateActive);

    // Run panel transiton
    _this._doTransition(oTab.panel, _this.options.animation, 'open', function() {
        // When finished, set active class to the panel
        oTab.panel.removeClass(_this.options.classes.stateDefault).addClass(_this.options.classes.stateActive);

        activeTab = (_this.getState() === 'accordion') ? oTab.accordionTab : oTab.tab;

        // And if enabled and a hash is present, scroll to the tab
        if(_this.options.scrollToActiveTab && hashIsPresent && (!_this._isInView(activeTab) || _this.options.animation !== 'default')) {
            scrollOffset = (_this.options.scrollToActiveTabOffsetElement) ? activeTab.offset().top - _this.options.scrollToActiveTabOffsetElement.height() : activeTab.offset().top;

            // Check if the animation option is enabled, and if the duration isn't 0
            if(_this.options.animation !== 'default' && _this.options.duration > 0) {
                // If so, set scrollTop with animate and use the 'animation' duration
                $('html, body').animate({
                    scrollTop: scrollOffset
                }, _this.options.duration);
            } else {
                //  If not, just set scrollTop
                $('html, body').scrollTop(scrollOffset);
            }
        }
    });

    this.$element.trigger('tabs-activate', oTab);
};

[herrschuessler]

Great. I'll close this PR in favour of your code. Would you issue a PR?

[ajmueller]

Sure thing. I'm not sure how quickly I will be able to get to it, especially considering my use-case here makes the assumption that the element for which offset gets compensated is assumed to be positioned at the very top and left of the viewport, which isn't necessarily the case.

@jellekralt what are your thoughts here? For scroll offsets, would you like to be able to compensate for elements that are not positioned at the top and left of the viewport? I would assume so. As I was building this out locally I noticed that this is a case we should account for:

[herrschuessler]

Wouldn't it make sense to assume that the offset should always be the position of the lower border of the scrollToAccordionOffsetElement - no matter wether that element is positioned at the top of the window or not?

I think you are already doing that in your example code...

[ajmueller]

Yep, that's what I was pointing out (that we should always scroll the user to the point where the active tab is visible just below the bottom of the fixed element). My example code doesn't take the scrollToAccordionOffsetElement's position into play, just its height. That can be seen when calculating the offset:

scrollOffset = (_this.options.scrollToActiveTabOffsetElement) ? activeTab.offset().top - _this.options.scrollToActiveTabOffsetElement.height() : activeTab.offset().top;

[herrschuessler]

@ajmueller your code is now in https://github.com/jellekralt/Responsive-Tabs/pull/77.

[ajmueller]

:+1: thanks @herrschuessler!

[jellekralt]

Hi Guys, Sorry for the late response guys, I haven't been able to spend as much time on the plugin as I'd like. Although I like to idea of an offset, I'd rather have a generic scrollToAccordionOffset option, in that way you don't only have the option to use an element as an offset but also other use other ways to calculate offsets.

That being said, I really appreciate the PR @herrschuessler, thanks a bunch! I'll accept it and make my changes.