search

PolymerElements / iron-fit-behavior

Fits an element into another element
17 stars 34 forks source link

vertical and horizontal offset affect top and left, not margin #59

Closed valdrinkoshi closed 8 years ago

valdrinkoshi commented 8 years ago

Fixes #54, fixes #55 by associating verticalOffset and horizontalOffset changes to affect top, left rather than margin-top + margin-bottom and margin-left + margin-right.

notwaldorf commented 8 years ago

I'm going to unassign myself from this, since @bicknellr is already on it :)

valdrinkoshi commented 8 years ago

ping @bicknellr :)

bicknellr commented 8 years ago

I think it might be useful to include in the documentation why these properties are better than just setting margin on the fitted element. Specifically, that verticalOffset / horizontalOffset apply only to the edge to which the fitted element is aligned. This lets you add space to the aligned edge of the fitted element and the positioning element without having to worry about what happens when dynamicAlign causes the fitted element to change alignment edges or when the fitted element's other edges are closer than verticalOffset / horizontalOffset of the fitInto element. tl;dr: As far as IronFitBehavior is concerned, verticalOffset / horizontalOffset can collapse but CSS margin does not. -- Is this correct? (It's been a bit since I last checked out this PR. :) )

valdrinkoshi commented 8 years ago

Not exactly, what happens is that margin, verticalOffset, horizontalOffset are never collapsed. But I kind of like the idea that "as IronFitBehavior is concerned, verticalOffset / horizontalOffset can collapse but CSS margin does not" :) I'll update it!

valdrinkoshi commented 8 years ago

@bicknellr updated, PTAL

bicknellr commented 8 years ago

Ah ok, I was just trying to make sure I understood the fix but if you'd rather do that and it still fixes the bug, then sgtm.

valdrinkoshi commented 8 years ago

seems like travis-ci is still having issues with tests on firefox. FWIW I've tested this locally and on saucelabs and tests are green on firefox. @bicknellr I'd go ahead and merge this, WDYT?

bicknellr commented 8 years ago

I think the original test case is still broken with the PR as of the current state: http://jsbin.com/xoqilu/edit?html,output

valdrinkoshi commented 8 years ago

It is working correctly: the black box is aligned to the bottom of the red box (positionTarget) with the right offset (+/-10), and doesn't render outside the window (fitInto)

bicknellr commented 8 years ago

Oh, I was looking at the bottom when vertical-align was -10 and thought it was a problem that it went outside the red box but this is what negative vertical-align is intended to do. Ok, LGTM.