-
Notifications
You must be signed in to change notification settings - Fork 425
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
Outlets: Add observers for controller element attributes #624
Outlets: Add observers for controller element attributes #624
Conversation
@marcoroth since you authored the original Outlets implementation, I'd really appreciate some code review on this. |
e23e112
to
93290ea
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.
Thanks @seanpdoyle for catching this oversight!
The outcome of this PR looks good to me. I left some comments and I'm curious to hear what you think.
In some cases, calls to `Element.setAttribute`, `Element.removeAttribute`, `Node.appendChild`, or `Element.remove` must be followed up with a call to `await this.nextFrame` so that Stimulus has an opportunity to synchronize. This commit introduces asynchronous helper method versions of those calls that bake-in the subsequent call to `this.nextFrame`.
With the current Outlet implementation, they're only ever connected or disconnected when an element _matching_ the outlet selector connects or disconnects. The selector _declared on the controller_ element can only ever be set once: when it's connected. If that attribute ever changes (for example, when it's set to a new value or removed entirely), the outlets are not updated. This commit adds test coverage to ensure the list of outlets and their items is synchronized with changes on both sides: when matching elements are connected and disconnected _as well as_ when the selector that dictates whether or not they match is added, updated, or removed. To do so, this commit extends the `SelectorObserver` to also manage the lifecycle of an `AttributeObserver` instance alongside its internally managed `ElementObserver`.
0178b27
to
971004a
Compare
e77a1d3
to
971004a
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.
This looks good me now! Thanks @seanpdoyle!
Extract
DomTestCase
helpersIn some cases, calls to
Element.setAttribute
,Element.removeAttribute
,Node.appendChild
, orElement.remove
mustbe followed up with a call to
await this.nextFrame
so that Stimulushas an opportunity to synchronize.
This commit introduces asynchronous helper method versions of those
calls that bake-in the subsequent call to
this.nextFrame
.Outlets: Add observers for controller element attributes
With the current Outlet implementation, they're only ever connected or
disconnected when an element matching the outlet selector connects or
disconnects. The selector declared on the controller element can only
ever be set once: when it's connected. If that attribute ever changes
(for example, when it's set to a new value or removed entirely), the
outlets are not updated.
This commit adds test coverage to ensure the list of outlets and their
items is synchronized with changes on both sides: when matching elements
are connected and disconnected as well as when the selector that
dictates whether or not they match is added, updated, or removed.
To do so, this commit extends the
OutletObserver
to also manage thelifecycle of an
AttributeObserver
instance alongside its internallymanaged
SelectorObserver
.