refactor(connected-position-strategy): use top/left instead of transforms for positioning #2024
Conversation
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
connected-position-top-left
branch
from
b5cc8e6 to
cdc7b70
Compare
|
|
||
| /** The offset in pixels for the overlay connection point on the x-axis */ | ||
| private _offsetX: number = 0; | ||
| private _x: number = 0; |
There was a problem hiding this comment.
Why rename these? They are still offset values.
There was a problem hiding this comment.
I guess they still looked pretty similar to the transforms. I'll put them back.
| // because it will need to be used for animations. | ||
| applyCssTransform(element, `translateX(${x}px) translateY(${y}px)`); | ||
| element.style.left = overlayPoint.x + 'px'; | ||
| element.style.top = overlayPoint.y + 'px'; |
There was a problem hiding this comment.
Should we not still round these, or do browsers simply ignore subpixel top/left?
There was a problem hiding this comment.
It doesn't look like it's an issue with the top/left properties.
src/lib/select/select.spec.ts
Outdated
| fixture.detectChanges(); | ||
|
|
||
| const overlayPane = overlayContainerElement.children[0] as HTMLElement; | ||
| overlayPane.style.position = 'absolute'; |
There was a problem hiding this comment.
It's because it doesn't look like we import all of the styles in the unit tests. This means that the overlay is position: static. It wasn't an issue with the transform approach since they work on all of the positions.
There was a problem hiding this comment.
Can you just add a comment mentioning that?
|
@kara should also take a look |
crisbeto
force-pushed
the
connected-position-top-left
branch
2 times, most recently
from
624dc07 to
f6c9ae4
Compare
|
Rebased and addressed the feedback. |
|
@kara will this conflict with your select alignment PR? |
|
Seems like a bunch of tests are failing on this one now. I'll take a better look into it tomorrow. |
…orms for positioning Switches the `ConnectedPositionStrategy` to use `top` and `left` for positioning, instead of `translateX` and `translateY`. This avoids having to do extra work to prevent subpixel rendering issues, while freeing up the transforms to be used on animations or for offsets.
crisbeto
force-pushed
the
connected-position-top-left
branch
from
f6c9ae4 to
e693fac
Compare
|
Tests are sorted out now. |
jelbourn
added
pr: lgtm
action: merge
|
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. |
4 participants
Switches the
ConnectedPositionStrategyto usetopandleftfor positioning, instead oftranslateXandtranslateY. This avoids having to do extra work to prevent subpixel rendering issues, while freeing up the transforms to be used on animations or for offsets.