feat(overlay): add connected position strategy #335
Conversation
googlebot
added
cla: yes
jelbourn
force-pushed
the
overlay-relative
branch
4 times, most recently
from
c72ed60
to
8bb60ad
Compare
|
|
||
|
|
||
| /** | ||
| * Gets the horizontal (x) "start"" dimension based on whether the overlay is in an RTL context. |
jelbourn
force-pushed
the
overlay-relative
branch
2 times, most recently
from
926d3ad
to
7704250
Compare
|
Added tests for |
jelbourn
force-pushed
the
overlay-relative
branch
13 times, most recently
from
9777480
to
0a8bfc8
Compare
|
Tests are green now. Swapped the browser setup to use iOS8 instead of iOS7 |
jelbourn
force-pushed
the
overlay-relative
branch
from
0a8bfc8
to
efa3f5c
Compare
| x = originStartX + (originRect.width / 2); | ||
| } else { | ||
| x = pos.originX == 'start' ? originStartX : originEndX; | ||
| } |
There was a problem hiding this comment.
Nitpick: You could split this into _getOriginConnectionX and _getOriginConnectionY methods to make it a bit more focused.
| let value = transformValue.trim(); | ||
|
|
||
| element.style.transform = value; | ||
| element.style.webkitTransform = value; |
There was a problem hiding this comment.
Do we need anything for moz here or is that covered by transform?
|
Done reviewing. Just a few typos and questions, but looks good to me. @hansl, @robertmesserle ? |
| @@ -41,7 +41,8 @@ export class Overlay { | |||
| constructor( | |||
| @Inject(OVERLAY_CONTAINER_TOKEN) private _overlayContainerElement: HTMLElement, | |||
| private _dynamicComponentLoader: DynamicComponentLoader, | |||
| private _appViewManager: AppViewManager) { | |||
| private _appViewManager: AppViewManager, | |||
| private _positionBuilder: OverlayPositionBuilder) { | |||
| } | |||
There was a problem hiding this comment.
Single line for { } for consistency?
jelbourn
force-pushed
the
overlay-relative
branch
11 times, most recently
from
0b1fbe7
to
ff3f8c4
Compare
jelbourn
force-pushed
the
overlay-relative
branch
from
ff3f8c4
to
dd69a53
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
R: @hansl @robertmesserle @kara
I'm still missing tests for
ViewportRuler, but I wanted to get the review started while I work on those.(and also see if this passes on CI)