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(connected-overlay): better handling of dynamic content #4250

Merged

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Apr 24, 2017

  • Refactors the ConnectedPositionStrategy to be able to use right and bottom to position the panel. This will allow the element to keep its position automatically, even if the element's size changes.
  • Removes the uses of the FakeViewportRuler in some of the unit tests since it doesn't work very well when the element is positioned relatively to the right edge of the screen.
  • Rounds down the values when testing positioning. This is necessary, because some browsers (particularly Chrome on Windows) can have some subpixel deviations.

Fixes #4155.

@crisbeto crisbeto self-assigned this Apr 24, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 24, 2017
@crisbeto crisbeto force-pushed the 4155/connected-pos-dynamic-content branch 6 times, most recently from 44eea9b to 098f44d Compare April 29, 2017 20:26
@crisbeto crisbeto changed the title wip: fix(connected-overlay): better handling of dynamic content fix(connected-overlay): better handling of dynamic content Apr 29, 2017
@crisbeto crisbeto assigned jelbourn and unassigned crisbeto Apr 29, 2017
@crisbeto
Copy link
Member Author

This one's ready to go now @jelbourn.

@@ -116,10 +117,11 @@ export class ConnectedPositionStrategy implements PositionStrategy {
// (top, left) coordinate for the overlay at `pos`.
let originPoint = this._getOriginConnectionPoint(originRect, pos);
let overlayPoint = this._getOverlayPoint(originPoint, overlayRect, viewportRect, pos);
let overlayDimensions = this._getCSSDimensions(overlayRect, overlayPoint, pos);
Copy link
Member

Choose a reason for hiding this comment

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

It's weird to have both overlayPoint and overlayDimensions since they're both kind of the same thing and it would be very easy to use the wrong one.

What if we leave the calculations they way they are now, but then also pass the ConnectionPositionPair and the overlay ClientRect to _setElementPosition which figures out if it should set bottom/right there? I think it might be better, since then you're deferring the calculation until you have to know and don't have to create a type to pass it around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. FWIW, I had it in a separate method, because the logic used to be a bit longer.

} else {
element.style[prop] = null;
}
});
Copy link
Member

@jelbourn jelbourn May 1, 2017

Choose a reason for hiding this comment

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

Rather than making a bunch of comments trying to communicate what I meant, I just tweaked the source a bit for comments and naming:

  private _setElementPosition(
      element: HTMLElement,
      overlayRect: ClientRect,
      overlayPoint: Point,
      pos: ConnectionPositionPair) {
    const  viewport = this._viewportRuler.getViewportRect();

    // We want to set either `top` or `bottom` based on whether the overlay wants to appear above
    // or below the origin.
    let verticalStyleProperty = pos.overlayY === 'bottom' ? 'bottom' : 'top';

    // When using `bottom`, we adjust the y position such that it is the distance
    // from the bottom of the viewport rather than the top.
    let y = verticalStyleProperty === 'top' ?
        overlayPoint.y :
        viewport.height - (overlayPoint.y + overlayRect.height);

    // We want to set either `left` or `right` based on whether the overlay wants to appear "before"
    // or "after" the origin. For the horizontal axis, the meaning of "before" and "after" change
    // based on whether the page is in RTL or LTR.
    let horizontalStyleProperty: string;
    if (this._dir === 'rtl') {
      horizontalStyleProperty = pos.overlayX === 'end' ? 'left' : 'right';
    } else {
      horizontalStyleProperty = pos.overlayX === 'end' ? 'right' : 'left';
    }

    // When we're setting `right`, we adjust the x position such that it is the distance
    // from the right edge of the viewport rather than the left edge.
    let x = horizontalStyleProperty === 'left' ?
      overlayPoint.x :
      viewport.width - (overlayPoint.x + overlayRect.width);


    // Reset any existing styles. This is necessary in case the preferred position has
    // changed since the last `apply`.
    ['top', 'bottom', 'left', 'right'].forEach(p => { element.style[p] = null; });

    element.style[verticalStyleProperty] = y;
    element.style[horizontalStyleProperty] = x;
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with some slight tweaks to the comments to mention that the property determines which direction the element will expand to.

@crisbeto crisbeto force-pushed the 4155/connected-pos-dynamic-content branch from 99f269b to 607e3a2 Compare May 1, 2017 20:36
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 1, 2017
@crisbeto crisbeto removed the action: merge The PR is ready for merge by the caretaker label May 1, 2017
@crisbeto crisbeto force-pushed the 4155/connected-pos-dynamic-content branch from 607e3a2 to ae7ebf3 Compare May 1, 2017 21:34
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label May 1, 2017
@andrewseguin
Copy link
Contributor

Needs rebase

@andrewseguin andrewseguin removed the action: merge The PR is ready for merge by the caretaker label May 2, 2017
* Refactors the `ConnectedPositionStrategy` to be able to use `right` and `bottom` to position the panel. This will allow the element to keep its position automatically, even if the element's size changes.
* Removes the uses of the `FakeViewportRuler` in some of the unit tests since it doesn't work very well when the element is positioned relatively to the right edge of the screen.
* Rounds down the values when testing positioning. This is necessary, because some browsers (particularly Chrome on Windows) can have some subpixel deviations.

Fixes angular#4155.
@crisbeto crisbeto force-pushed the 4155/connected-pos-dynamic-content branch from ae7ebf3 to ad153c2 Compare May 3, 2017 19:09
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label May 3, 2017
@crisbeto
Copy link
Member Author

crisbeto commented May 3, 2017

Rebased @andrewseguin.

@andrewseguin andrewseguin merged commit 525ce1e into angular:master May 4, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectedPositionStrategy doesn't work properly if the overlay's content is dynamic
4 participants