Allowing resize for fully below the fold elements even when top margin changed #8250
Conversation
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to dvoytenko jridgewell
/to alanorozco camelburrito chenshay choumx cvializ ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
|
/cc @zhouyx |
src/service/resources-impl.js
Outdated
| @@ -1095,8 +1095,9 @@ export class Resources { | |||
| // 3. Active elements are immediately resized. The assumption is that | |||
| // the resize is triggered by the user action or soon after. | |||
| resize = true; | |||
| } else if (topMarginDiff == 0 && box.bottom + Math.min(heightDiff, 0) >= | |||
| viewportRect.bottom - bottomOffset) { | |||
| } else if (box.top >= viewportRect.bottom - bottomOffset || | |||
There was a problem hiding this comment.
This makes sense.
But I have two questions: when get box.top is its margin included?
What happens when topMarginDiff is a negative value? Or can it be a negative value?
There was a problem hiding this comment.
Good spot. You're right box.top does not include the margin. I've refined this so we calculate the top boundary that isn't changed and then check that this is below the fold.
With my refined approach, topMarginDiff being negative is not an issue, but I guess there is a potential issue if someone tries to set a top margin to be -100 or something. I've filed #8333 and will address this in a separate PR.
Fixes something that was missed in #6824. If an element is fully below the fold, then resize should be allowed even when top margin is being changed, since changes to the top boundary won't be visible to the user.