Improve editor to prevent layout breakage - #14
Conversation
schmijos
left a comment
schmijos
left a comment
There was a problem hiding this comment.
The system tests are still missing? How do you prove to me that "it still all works"?
| params.append("template[render_path][]", p), | ||
| ); | ||
|
|
||
| return `/ferbe/template/edit?${params.toString()}`; |
There was a problem hiding this comment.
Yes, I am aware. I plan to implement this together with the changes to the generator in TICKET-25969 later
I wanted to wait for your approval of the new approach first. Also, there's a separate Ticket (TICKET-25972). Should I add them In a separate PR first? or in this one? or add them later? |
|
Let's do them tests separately. |
schmijos
left a comment
schmijos
left a comment
There was a problem hiding this comment.
Nice, I happy with that approach! Some minor improvement suggestions, otherwise good to merge.
| params.append("template[path]", template.filePath); | ||
| template.renderPath.forEach((p) => | ||
| params.append("template[render_path][]", p), | ||
| ); |
There was a problem hiding this comment.
What about a more declarative approach?
const params = new URLSearchParams([
["url", url],
["template[path]", template.filePath],
...template.renderPath.map((p) => ["template[render_path][]", p] as const),
]);| .then((r) => r.text()) | ||
| .then((html) => { | ||
| Turbo.renderStreamMessage(html); | ||
| window.history.pushState({}, "", editorUrl(template)); |
There was a problem hiding this comment.
Why are you fiddling with the history here?
There was a problem hiding this comment.
So the history/url reflects the current editor state.
But it is buggy and when navigating the render path it also doesn't get updated.
I will remove it.
| .then((r) => r.text()) | ||
| .then((html) => Turbo.renderStreamMessage(html)) | ||
| .catch((err) => console.error("Failed to open editor:", err)); | ||
| window.top.dispatchEvent( |
There was a problem hiding this comment.
That's an interesting idea. I wouldn't have thought of that but used postMessage instead. But you're totally right. Cross-origin communication is not needed.
| params.append("url", template.url); | ||
| params.append("template[path]", template.filePath); | ||
| template.renderPath.forEach((p) => | ||
| params.append("template[render_path][]", p), | ||
| ); |
There was a problem hiding this comment.
Second time occurrence of this pattern.
2 participants
TICKET-25970
Now, opening a template will open a completely new page containing the content as an
iframeas well as the editor.This also allowed me to remove the error handling logic, as now the templates can still be edited with errors.
Opening the editor now works using a custom JS event that will open the whole new page if not opened yet. Otherwise, it will replace the editor using a turbo stream.
Other changes:
hostfor the host application andapplicationfor the engine views.display: content;to make the custom elements less invasive (can be seen with the newly added "grid" in the dummy app)