fix(menu): unable to bind to xPosition and yPosition #4213
Conversation
googlebot
added
the
cla: yes
|
Fixes #4169 |
src/lib/menu/menu-directive.ts
Outdated
| constructor(@Attribute('xPosition') posX: MenuPositionX, | ||
| @Attribute('yPosition') posY: MenuPositionY, | ||
| @Attribute('x-position') deprecatedPosX: MenuPositionX, | ||
| constructor(@Attribute('x-position') deprecatedPosX: MenuPositionX, |
There was a problem hiding this comment.
While we're in here, let's remove the deprecated positions?
src/lib/menu/menu-directive.ts
Outdated
| @@ -81,6 +100,12 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy { | |||
| } | |||
| } | |||
|
|
|||
| ngOnChanges(changes: SimpleChanges) { | |||
| if (changes['xPosition'] || changes['yPosition']) { | |||
| this.setPositionClasses(); | |||
There was a problem hiding this comment.
Curious why we are adding a hook to call setPositionClasses when we already have the setters above. I think it might be cleaner not to break up the logic?
There was a problem hiding this comment.
Not sure I understood the question. The setters aren't dealing with the classes at all.
There was a problem hiding this comment.
The classes need to be updated whenever the xPosition or the yPosition changes. For that reason, it makes more sense to me to call this method in the xPosition and yPosition setters rather than creating a hook just to listen for changes on those properties.
crisbeto
force-pushed
the
4169/menu-position-binding
branch
2 times, most recently
from
582c2a3
to
e14d91d
Compare
|
Addressed the feedback @kara. |
Fixes not being able to use data bindings on the `xPosition` and `yPosition` properties. **Note:** These changes could be considered breaking, if somebody was implementing the `MdMenuPanel` interface. Fixes angular#4169.
crisbeto
force-pushed
the
4169/menu-position-binding
branch
from
e14d91d
to
b6af4d0
Compare
|
Also fixes #1329 |
kara
added
pr: lgtm
action: merge
mmalerba
added
the
presubmit failures
|
There are some teams in google that extend MdMenuPanel, we will need to update them before merging |
andrewseguin
removed
the
presubmit failures
|
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. |
Fixes not being able to use data bindings on the
xPositionandyPositionproperties.Note: These changes could be considered breaking, if somebody was implementing the
MdMenuPanelinterface.Fixes #4169.