Introduce FrameElement.getElementById(id: string)
#1136
Conversation
| @@ -162,9 +162,9 @@ export class Session { | |||
| const frameTarget = element.getAttribute("data-turbo-frame") | |||
| const frame = frameTarget == "_top" ? | |||
| null : | |||
| document.getElementById(frameTarget) || findClosestRecursively(element, "turbo-frame:not([disabled])") | |||
| FrameElement.getElementById(frameTarget) || findClosestRecursively(element, "turbo-frame") | |||
There was a problem hiding this comment.
This looks like it could change the logic – e.g. a nested frame situation where the nearest frame is disabled.
In the old code, the nearest frame would be skipped and the outer frame returned, in the new logic nearest frame would be returned and the subsequent conditional would fail.
There was a problem hiding this comment.
| FrameElement.getElementById(frameTarget) || findClosestRecursively(element, "turbo-frame") | |
| FrameElement.getElementById(frameTarget) || findClosestRecursively(element, "turbo-frame:not([disabled])") |
|
|
||
| if (isUnsafe || isStream || frame instanceof FrameElement) { | ||
| if (isUnsafe || isStream || (frame && !frame.disabled)) { |
There was a problem hiding this comment.
@sfnelson since FrameElement.getElementById accounts for instanceof FrameElement, does this property check account for the concerns you've shared in https://github.com/hotwired/turbo/pull/1136/files/b750a8f2e44ef3133baf6328e5187c80ae5cf2e1#r1496721094?
There was a problem hiding this comment.
For the sake of shrinking the diff, both of these lines could likely be reverted in part:
| if (isUnsafe || isStream || (frame && !frame.disabled)) { | |
| if (isUnsafe || isStream || frame instanceof FrameElement) { |
There was a problem hiding this comment.
Yeah, that looks fine.
Would it make sense to add a static helper for finding the enclosing (non-disabled) FrameElement? In that case the findClosestRecursively could be replaced by e.g. FrameElement.findEnclosingFrame(element) and the check would become if (isUnsafe || isStream || frame) {.
There was a problem hiding this comment.
For the purposes of code reuse, I can see two additional utility methods that would be helpful:
FrameElement.findEnclosingFrame(element)would walk up the tree to find the enclosing frame (if any)FrameElement.findTargetFrame(element)would expose#findFrameElement– given an element it would identify which frame should be targeted – for use with links/forms that target frames for navigation
I think these would bring general utility to downstream consumers of Turbo – both of these cases have come up in our use of Frames to implement modals.
The `FrameElement.getElementById(id: string)` static method unifies the internal logic to locate a `<turbo-frame>` element by its `[id]`, while providing that consistent behavior to consumer applications.
seanpdoyle
force-pushed
the
frame-element-get-by-element-id
branch
from
b750a8f
to
3f3d99c
Compare
The
FrameElement.getElementById(id: string)static method unifies the internal logic to locate a<turbo-frame>element by its[id], while providing that consistent behavior to consumer applications.