-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix(modal): restore broken stacked modals #6104
Conversation
// This might need to be marked as a breaking change because | ||
// "$$topModalIndex" could eventually shadow a property with the | ||
// same name defined on the provided modal scope. | ||
modal.scope.$$topModalIndex = topModalIndex; |
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.
i'm ok w/ the name seeing as one is not supposed to be using $
as prefixes for their code. but this is good insight to be aware of.
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.
i don't think it needs a BC because of that reason.
@fpipita, i'm ok with this. thanks for putting this together. |
@icfantv please, let me know whether I should remove the BC tag from the commit message. |
That's fine, or I can remove it on commit. |
Ok, done. |
@fpipita did you forget to push your changes? i'm not seeing them. |
@icfantv I've pushed them, the BC tag is not in the commit body anymore. Did I get anything wrong? |
My bad, I was looking at the comment in the code. Do you mind fixing that please? |
No problem, I've cleaned it up. |
@icfantv , do you mind making a new release (2.0.1) with this fix please? I'm waiting for it to publish a website that uses version 2.0.0 and has the stack modals bug. |
Done. |
Hi,
with 2.0.0, stacked modals are not getting the
zIndex
property correctly set anymore because theindex
property needed byngStyle
to compute thezIndex
property, is not available on thengStyle
scope (which is the modal scope).This PR solves it by simply adding a new "private" property to the modal scope,
$$topModalIndex
which provides the actual modal index value.Closes #6103