feat(cdk/overlay): Allow passing separate X and Y values for the view… #29563
Conversation
LukeyBeachBoy
requested review from
crisbeto and
wagnermaciel
and removed request for
a team
LukeyBeachBoy
force-pushed
the
feature-cdk-overlay-split-viewport-margin
branch
2 times, most recently
from
37b5e76 to
0903d6e
Compare
Hi, done :) |
LukeyBeachBoy
force-pushed
the
feature-cdk-overlay-split-viewport-margin
branch
from
0903d6e to
2a78c90
Compare
|
Hi @crisbeto, could I get another review? Thanks! |
| * Sets a minimum distance the overlay may be positioned from the left edge of the viewport. | ||
| * @param margin Required margin between the overlay and the viewport edge in pixels. | ||
| */ | ||
| withViewportMarginLeft(margin: number): this { |
There was a problem hiding this comment.
A couple of notes:
- Instead of introducing properties for each direction, we should just change the viewport margin to be something like
number | {top: number, bottom: number, start: number, end: number}. - We should use
startandend, instead ofleftandright, because they get inverted in RTL.
There was a problem hiding this comment.
Good points, I've updated the parameter to a union type as suggested as well as renamed left/right -> start/end
LukeyBeachBoy
force-pushed
the
feature-cdk-overlay-split-viewport-margin
branch
from
2a78c90 to
742668a
Compare
LukeyBeachBoy
force-pushed
the
feature-cdk-overlay-split-viewport-margin
branch
3 times, most recently
from
6804076 to
73539f2
Compare
|
Hi @crisbeto, I updated my tests and everything should build correctly now. Could you review again when you have time? |
| @@ -0,0 +1 @@ | |||
| export type ViewportMargin = number | {top?: number; bottom?: number; start?: number; end?: number}; | |||
There was a problem hiding this comment.
It looks like this file is missing a license header. You can also put the type in one of the existing files.
…portMargin The overlay directive now accepts two additional (optional parameters) [viewportMarginX] and [viewportMarginY]. You can use these to pass separate margin values for the viewport.
LukeyBeachBoy
force-pushed
the
feature-cdk-overlay-split-viewport-margin
branch
from
73539f2 to
70450b2
Compare
|
@crisbeto Right, sorry. I've moved it to another file now |
|
Is there something else I need to change for this PR? I see the merge status and internal tests are failing but I'm unable to see any details |
There was a problem hiding this comment.
It looks like this broke some internal screenshot tests, but I haven't had the time to debug them. In some of them an autocomplete that is close to the viewport edge was the same width as the trigger, but after the change it's slightly smaller. I suspect the margin might be getting doubled somewhere where previously it wasn't.
adolgachev
added
requires: TGP
| } else if (isBoundedByRightViewportEdge) { | ||
| left = origin.x; | ||
| width = viewport.right - origin.x; | ||
| width = viewport.right - origin.x - this._getViewportMarginEnd(); |
There was a problem hiding this comment.
I think the screenshot diffs @crisbeto mentioned are caused by this line. It previously wasn't accounting for the margin at all in this case, and now it is. This seems like probably just a bug that nobody noticed before so I'm inclined to approve the screenshots
mmalerba
added
target: patch
|
Thanks for the approval! |
…portMargin
The overlay directive now accepts two additional (optional parameters) [viewportMarginX] and [viewportMarginY]. You can use these to pass separate margin values for the viewport.