feat(overlay): CoverPositionStrategy #15691
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * @license | ||
| * Copyright Google LLC All Rights Reserved. | ||
| * | ||
| * Use of this source code is governed by an MIT-style license that can be | ||
| * found in the LICENSE file at https://angular.io/license | ||
| */ | ||
|
|
||
| import {Inject, Injectable} from '@angular/core'; | ||
| import {DOCUMENT} from '@angular/common'; | ||
| import {Platform} from '@angular/cdk/platform'; | ||
| import {ViewportRuler} from '@angular/cdk/scrolling'; | ||
| import {FlexibleConnectedPositionStrategyOrigin} from './flexible-positioning'; | ||
| import {CoverPositionStrategy} from './cover-position-strategy'; | ||
|
|
||
| @Injectable({providedIn: 'root'}) | ||
| export class CoverPositionStrategyFactory { | ||
|
There was a problem hiding this comment. Why do we need a dedicated factory for it? All of the other position strategies are created through the There was a problem hiding this comment. IMO all of the strategies should move towards a separate factory approach for better tree shakability. In this case, I did not want to add something new that would not be tree-shaken away by default. There was a problem hiding this comment. I've had that on my list for a while, but the problem is that we can't do it easily without either a breaking change or having to maintain two approaches for a while. It would also be tricky to migrate people once we remove the deprecated approach. Having only this strategy with a dedicated factory is inconsistent. There was a problem hiding this comment. Probably you'd add individual factories, have the existing combined factory use them for now, mark it as deprecated and remove it after a couple releases? I'll defer to whatever Jeremy prefers though. |
||
| constructor( | ||
| private _viewportRuler: ViewportRuler, | ||
| @Inject(DOCUMENT) private _document: any, | ||
| private _platform: Platform) {} | ||
|
|
||
| create() { | ||
| return this.createWithConnections({}); | ||
| } | ||
|
|
||
| createWithConnections({top, right, left, bottom}: { | ||
| top?: FlexibleConnectedPositionStrategyOrigin, | ||
| right?: FlexibleConnectedPositionStrategyOrigin, | ||
| bottom?: FlexibleConnectedPositionStrategyOrigin, | ||
| left?: FlexibleConnectedPositionStrategyOrigin | ||
| }) { | ||
| return new CoverPositionStrategy( | ||
| this._viewportRuler, | ||
| this._document, | ||
| this._platform, | ||
| top, | ||
| right, | ||
| bottom, | ||
| left); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add JsDoc description
I'd also add a file-level comment that explains that this class exists so that people can instantiate a
CoverPositionStrategywithout manually passing in its numerous injected depsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do - IMO all of the factories for position strategies should move to something like this rather than baking them into Overlay (and defeating tree-shaking of them in the process).