refactor(overlay): allow for object to be passed to the OverlayState constructor #6615
Conversation
crisbeto
requested review from
andrewseguin,
jelbourn,
josephperrott,
kara and
mmalerba
as code owners
googlebot
added
the
cla: yes
| @@ -53,6 +53,12 @@ export class OverlayState { | |||
| /** The direction of the text in the overlay panel. */ | |||
| direction?: Direction = 'ltr'; | |||
|
|
|||
| constructor(state?: OverlayState) { | |||
| if (state) { | |||
There was a problem hiding this comment.
Could you do this?
extendObject(this, state);(from core/util, handles null / undefined)
There was a problem hiding this comment.
I didn't use extendObject, because it would introduce a dependency between Material and the CDK.
There was a problem hiding this comment.
Ah, I forgot that was in material.
| @@ -272,7 +272,12 @@ export class ConnectedOverlayDirective implements OnDestroy, OnChanges { | |||
|
|
|||
| /** Builds the overlay config based on the directive's inputs */ | |||
| private _buildConfig(): OverlayState { | |||
| let overlayConfig = new OverlayState(); | |||
| const positionStrategy = this._position = this._createPositionStrategy(); | |||
| const overlayConfig = new OverlayState({ | |||
There was a problem hiding this comment.
Since all of the options are optional now, could you just change most places to, e.g.,
const overlayConfig = {
positionStrategy,
scrollStrategy: this.scrollStrategy,
hasBackdrop: this.hasBackdrop,
};?
There was a problem hiding this comment.
That's what I did. The position in this particular case is different, because we want to save a reference to it before we pass it to the OverlayRef.
There was a problem hiding this comment.
I meant omitting the new OverlayState( part and just having the object literal
There was a problem hiding this comment.
Ah, gotcha. Instantiating the class lets us apply the default options for things like the direction, backdrop class and scroll strategy.
src/cdk/overlay/overlay.spec.ts
Outdated
|
|
||
| state.hasBackdrop = true; | ||
|
|
||
| let state = new OverlayState({ hasBackdrop: true }); |
Adds a rule to enforce that we have consistent spacing inside curly braces and fixes any violations. Relates to angular#6615 (comment).
Adds a rule to enforce that we have consistent spacing inside curly braces and fixes any violations. Relates to angular#6615 (comment).
…constructor Allows for a config object to be passed to the `OverlayState` constructor. This makes the longer configurations a bit more convenient to use.
crisbeto
force-pushed
the
overlay-state-obj
branch
from
b50084b to
e325ca3
Compare
Adds a rule to enforce that we have consistent spacing inside curly braces and fixes any violations. Relates to angular#6615 (comment).
|
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. |
Allows for a config object to be passed to the
OverlayStateconstructor. This makes the longer configurations a bit more convenient to use.