-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(drag-drop): allow for element in DropListRef to be changed #15091
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(drag-drop): allow for element in DropListRef to be changed #15091
Conversation
@crisbeto I saw that you removed the readonly allowed direct swap of the element... I suggest to use the same approach used in While it doesn't look like any special teardown or init is required for this element, it might be so in the future. Using |
@shlomiassaf the reason I didn't do it was because the element is accessed by the |
2394ffc
to
3d12a8b
Compare
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.
LGTM
@crisbeto passes presubmit, needs rebase |
It doesn't look like there's a good reason to have the `element` be readonly, aside from minimizing the coercion logic, since we only use the element in a handful of places and we don't bind any events to it so we don't need any cleanup logic. These changes allow for the element to be swapped out by removing the `readonly`. Fixes angular#15086.
Rebased. |
3d12a8b
to
87aa831
Compare
@crisbeto can you please fix the error on this PR? |
The |
…ar#15091) It doesn't look like there's a good reason to have the `element` be readonly, aside from minimizing the coercion logic, since we only use the element in a handful of places and we don't bind any events to it so we don't need any cleanup logic. These changes allow for the element to be swapped out by removing the `readonly`. Fixes angular#15086.
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. |
It doesn't look like there's a good reason to have the
element
be readonly, aside from minimizing the coercion logic, since we only use the element in a handful of places and we don't bind any events to it so we don't need any cleanup logic. These changes allow for the element to be swapped out by removing thereadonly
.Fixes #15086.