- Issue created by @lauriii
- Assigned to gauravvvv
- Status changed to Postponed
5 months ago 8:15am 19 July 2024 - ๐ซ๐ฎFinland lauriii Finland
We probably need โจ Contextual form values need to be integrated with Redux Active before this can be worked on.
- Issue was unassigned.
- ๐ซ๐ฎFinland lauriii Finland
An additional consideration for improving performance is to only re-render the parts that were changed. There's a similar improvements in the works for Layout Builder: โจ Only reload updated part of Layout Builder element Needs work .
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
+1, that seemed an evident implied requirement. ๐
- ๐ฌ๐งUnited Kingdom catch
One thing about #5/6, the layout builder issue is trying to scale down from the entire layout to a block.
However theoretically we could scale down to an individual prop (i.e. one field value, one static prop etc.) within a component. Would require being able to identify exactly what has changed vs not (possibly down to field columns) and also being able to replace the HTML at that level of granularity.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
#7:
However theoretically we could scale down to an individual prop (i.e. one field value, one static prop etc.) within a component.
Yep.
Would require being able to identify exactly what has changed vs not (possibly down to field columns) and also being able to replace the HTML at that level of granularity.
Yep.
But if it worked, it would be less for the browser to re-render etc.
Yep.
Happy to say that we're way ahead of you on this one: ๐ฑ [META] Real-time preview: supporting back-end infrastructure Needs work ๐
- ๐ซ๐ฎFinland lauriii Finland
This will be at least partially solved by โจ Contextual form values need to be integrated with Redux Active based on @bnjmnm.
- Assigned to lauriii
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Actually, having seen โจ Contextual form values need to be integrated with Redux Active in action, I think this is already implemented there? ๐
@lauriii, do you want to rescope this issue to handle #5 specifically? Because right now, the entire preview is updated โ i.e. the entire component tree is re-rendered.
So I think rescoping this to would make sense.
(The even more efficient thing @catch is talking about in #7 requires the AST work in ๐ฑ [META] Real-time preview: supporting back-end infrastructure Needs work to be done, what I'm proposing here would be an easily achievable intermediary step.)
- Issue was unassigned.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Issue rescoped ๐
Crafted a fairly detailed proposed resolution.
- Status changed to Active
5 months ago 2:59pm 26 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
โจ Contextual form values need to be integrated with Redux Active is in! ๐ฅณ
- ๐บ๐ธUnited States effulgentsia
extract the subtree of layout + model that represents only the component the Content Creator just modified
send that to /api/preview aka \Drupal\experience_builder\Controller\SdcController::preview() (which is agnostic to this โ no code changes needed on the server side ๐)I think we should consider doing this as a JSON PATCH request. We could either generate the json patch payload ourselves (e.g., by doing the extraction as proposed above), or we could use a library like JSON-Patch, which can generate a patch payload either by comparing two full objects, or a full object and an observer object.
This could also be an ingredient in letting the server efficiently maintain a stack of mini-revisions (like an auto-save feature) in between full entity revisions (user clicks a Save button).
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
#14: I respectfully disagree.
Because:
- extract the subtree of
layout
+model
that represents only the component the Content Creator just modified
๐this is already implemented in
DummyPropsEditForm.tsx
'sfindNodeInObjectTree()
>1 month ago, for being able to edit the selected component's props. It's orders of magnitude simpler than what you describe ๐Maybe we eventually end up using something like JSON PATCH, but I don't see how it is relevant nor appropriate for the "client sends JSON blob in request to server", "server responds with markup"?
- extract the subtree of
- ๐บ๐ธUnited States effulgentsia
I don't see how it is relevant nor appropriate for the "client sends JSON blob in request to server", "server responds with markup"?
Fair enough. It's more relevant for auto-save: I make a change and the server keeps that state around (in something like a pending revision, though not necessarily literally a pending revision) without me needing to click a Save button. But I'll open a separate issue for that.
There's some overlap between: I made a small change and want the server to know about it so that I get an updated preview, and I made a small change and want the server to know about it so that it can save that state. And at some point it would probably make sense to do both in a single HTTP request rather than need two different (and differently formatted) round trips. But it's two different use cases, so for now we can work on them in different issues.
- ๐ฌ๐งUnited Kingdom catch
There's already https://www.drupal.org/project/autosave_form โ for entity forms, I discussed that module with @hchonov because in my head (prior to that conversation) I'd always thought it should worth with entity revisions. However autosave_form saves form state instead, and there are multiple good reasons for that:
1. It's consistent before and after the entity is saved.
2. There is no need to suppress or otherwise deal with validation of the entity.
3. Because it doesn't interfere with entity revisions, it's completely agnostic of modules like content_moderation and workspaces.
I really don't think it's a good idea for experience builder to implement its entire own autosave system from whole cloth, without at least fully exploring this first.
- ๐บ๐ธUnited States effulgentsia
+1 to #17. I think the "submit form" part we might want to do differently, but otherwise +1 to reusing a good chunk of Autosave Form.
- ๐ซ๐ฎFinland lauriii Finland
You may already be aware of this but to be sure, I want to to point out that Autosave Form does not currently have the desired user experience. With Autosave Form, autosave states are only displayed for a single user. That does not match the user experience that majority of platforms designed for collaboration are taking, and therefore I don't think it's sufficient for us. On majority of platforms that I've benchmarked, autosave is the way to save draft revisions. This means there isn't a separate action needed for saving content as a draft revision. Users can then mark the content either ready for review of published depending on their workflow.
Also, the saving of content every 60 seconds or some another interval unless it's something very low is not good enough for user experience. We need save to happen as soon as changes are being made (probably at a similar speed which we update the preview). People may make a change quickly and just close their laptop which is how people work on Google Docs, etc.
That said, my concern is on the user experience, not the module. I'm agnostic to using parts of the pre-existing module so long as we can get the desired user experience.
- ๐ฌ๐งUnited Kingdom catch
On majority of platforms that I've benchmarked, autosave is the way to save draft revisions. This means there isn't a separate action needed for saving content as a draft revision.
This is how I originally thought about the problem before talking to @hchonov, especially for collaborative editing, and yeah I still think that can work once there is a 'viable' entity to save
Additionally, to be able to share a link to collaboratively work on something, it would need to exist with a URI, which again in the absence of not-yet-existing alternatives means a draft entity revision.
Workspaces provides all the plumbing needed for this, not the UX or the autosave of course. There would need to be a way to then delete/prune the temporary revisions, which is would be easy enough to add because they'll be linked to a workspace.
However the big limitation of this is that to save a draft revision, it needs to be in such a state (title and other required fields) that it can conceivably be published and passes entity validation.
If we try to save draft revisions that are not valid/viable, we would run into a huge set of problems, which would then take us back to form state (or some other representation of the form as opposed to the entity). It might be we want form state for the 'invalid' state and entity revisions for the 'draft'/saveable state but we should try to get @hchonov on here.
- ๐บ๐ธUnited States effulgentsia
@tedbow helpfully opened ๐ฑ Research: Possible backend implementations of auto-save, drafts, and publishing Active for discussing/brainstorming auto-save and manually saving drafts or published revisions.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
I doubt this is still critical now that @jessebaker's ๐ Remove flickering when preview is being updated Fixed achieved flicker-free updates?
And if we'd build the AST-based updating in ๐ฑ [META] Real-time preview: supporting back-end infrastructure Needs work , it likely would literally be unnecessary. It might then actually be more of an optimization for server-side scalability and high-latency clients.
(It might then still be helpful for cases where AST-based updates are impossible, i.e. when there's logic in the rendering, which will be the case for non-SDC Components, such as Blocks. Still, this then should be worked on much later!)
@jessebaker: would you agree with downgrading this to ? ๐
- ๐บ๐ธUnited States effulgentsia
In addition to downgrading this from Critical (to either Normal or Major), I also think we should consider postponing it until after we've implemented AutoSave in ๐ฑ Research: Possible backend implementations of auto-save, drafts, and publishing Active . It's not that we can't work on this until then, but I think it would be more efficient to wait until that's done and there's plenty of other front-end things that need doing in the meantime.
- ๐ฌ๐งUnited Kingdom jessebaker
Agreed. I've moved it to Normal priority.
- ๐ฌ๐งUnited Kingdom jessebaker
Postponed pending ๐ฑ Research: Possible backend implementations of auto-save, drafts, and publishing Active