feat: menu #258
feat: menu #258
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
looks good to me, nice work 👍 |
|
Please summarize if there are any blockers on your end. If not, let's merge and move forward. Can you create tasks for the required improvements? |
|
No blockers on my side. I was just pointing out quite important thing to always remember to run |
| if (this.enablePositions) { | ||
| const prevMenuPosition = this.cdkTrigger.menuPosition; | ||
| this.cdkTrigger.menuPosition = [positions]; | ||
| this.fireNgOnChanges('menuPosition', this.cdkTrigger.menuPosition, prevMenuPosition); |
There was a problem hiding this comment.
I've updated CDK to 19.1, and now there's a public ngOnChanges method :)
At the same time, I moved the effect to the constructor (based on the discussion in the progress bar).
Could you check if everything looks correct?
There was a problem hiding this comment.
I see it, it has been added for over one month
In regard to the effect we an simply create a method on the class per each effect that is being run in the constructor. I see this as a more consistent approach (eslint will not complain that there is an unused prop when haiving a prop per each effect). I go this way in popover, hover card and tooltip.
There was a problem hiding this comment.
it was released on 19.1
angular/components#30234
https://github.com/angular/components/releases/tag/19.1.0
And since this was the 19.1 release, the discussion above initially confused me a bit—how to proceed without access to the method :)
I've made further adjustments to the code.
There was a problem hiding this comment.
Ok. I said that a few times that this is necessary when the method is implemented. I believe if we read our comments carefully, there will be less confusion.
No description provided.