Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #224 and #857 - toolbar in absolute container #1152

Merged
merged 7 commits into from
Jul 21, 2016

Conversation

linkesch
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
New tests added? not needed
Fixed tickets #224, #857
License MIT

Description

Fix positioning of a toolbar and an anchor preview when elementsContainer has position: absolute or position: fixed.

If elementsContainer is absolute or fixed, it will calculate positions relative to this element instead of window.

I also added a demo demo/absolute-container.html which demonstrates this behavior.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage decreased (-0.7%) to 94.184% when pulling a5ea3df on fix-toolbar-in-absolute-container into d57e4b2 on master.

positions.top += elementsContainer.scrollTop;
// If container element isn't absolute / fixed, adjust top position according to window scroll position
} else {
positions.top += this.window.pageYOffset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the calculation of the adjustment of top be done within the if(elementsContainerAboslute) block above?

Something like:

var topAdjustment = this.window.pageYOffset;

 // If container element is absolute / fixed, recalculate boundaries to be relative to the container
 if (elementsContainerAbsolute) {
    // other existing logic...

    topAdjustment = elementsContainer.scrollTop;
}

middleBoundary = boundary.left + boundary.width / 2;
positions.top = (boundary.top - toolbarHeight) + topAdjustment;

That way we can have one less if-condition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll do that

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage decreased (-0.3%) to 94.652% when pulling 72055ea on fix-toolbar-in-absolute-container into d57e4b2 on master.

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+0.05%) to 94.974% when pulling 97223ab on fix-toolbar-in-absolute-container into d57e4b2 on master.

@nmielnik nmielnik merged commit 77e941a into master Jul 21, 2016
@j0k3r j0k3r deleted the fix-toolbar-in-absolute-container branch July 22, 2016 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants