- Issue created by @lauriii
- Status changed to Needs review
5 months ago 8:15am 19 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
The fundamental question here:how will the client be able to update the preview in real-time, because it needs to know how to take those form input values (
<input>
,<select>
, etc) and transform them into the values that must be fed into the components.The only way I see to avoid needing a round trip to get the SDC prop value for a particular form is to hardcode some knowledge about the most commonly used field widgetsโ forms to be able to avoid that round trip.
But that still wonโt eliminate the round trip for updating the preview, because the rendering of components also needs a round trip. So it's not that big of a deal.
@bnjmnm, you having built ๐ Add component instance edit form to contextual panel RTBC , are best positioned to answer this. Do you agree with at least starting with a round trip?
- ๐ซ๐ฎFinland lauriii Finland
FYI, I opened โจ Partial preview updates: update preview of modified component only, not entire component tree Active which is focused on handling the updates to the preview.
- ๐ฌ๐งUnited Kingdom catch
This is not really my area but I did have one idea about the round trips.
Because this is form data-esque, it would be tempting to use a POST here, but can we use a GET instead (either an AJAX get, or whatever fancy endpoint instead of a Drupal AJAX endpoint), and then also return private but cacheable headers to the browser (or maybe local storage would work).
This would have a couple of impacts:
If this ever gets used for default values, the response should be entirely cacheable including across different content/forms for the same user.
If someone is editing any kind of text field and uses the backspace key, the previous preview (i.e. with the content they're going back to) should be browser cached already, so could save a round trip in that case.
There would need to be some kind of limit, about 2000 chars usually, to the GET request, so at some point it's going to hit a limit, but maybe it can switch between GET and POST depending on URL length even.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
In discussing this with @jessebaker, ๐ Implement front-end (React) routing library Needs work would ideally land first, because both will be touching the same areas of the codebase.
EDIT: cross-posted with @catch in #4 โ we were thinking along similar lines ๐๐
So then why keep the client-side model?
to allow for actual real-time updating of the preview in the future, when:- we have the full AST (see ๐ฑ [META] Real-time preview: supporting back-end infrastructure Needs work , and particularly the proposed end state of that), which means we'd be technically able to avoid the round trip to the server for updating the component preview โ at least in cases where no Twig extensions/functions are used in an SDC's Twig template
- we have pure client-side widgets at some undefined point in the future, because ๐ Evolve component instance edit form to become simpler: generate a Field Widget directly Fixed and its predecessor do not mean we can't do that, they mean that we should make it possible to use the existing rich ecosystem of Drupal field widget plugins
When both of those are done, the client-side model will be crucial to have. (Plus we also need it for undo functionality.)
- ๐ฌ๐งUnited Kingdom catch
One thing with the latency. I recently had a laptop issue where my laptop was massively overheating, which would result in the CPU being throttled by linux (yes it got that hot). This made everything slow, but it was worst in google docs - because you edit the content directly where it is rendered, there must be some kind of server round trip before you see what you've typed for collaborative editing etc, and that can result in massive lag. Searching for 'google docs lag' finds lots of similar reports.
If we're editing in off canvas with a preview in the main pane, then when people are actually typing etc. they won't be looking directly at the content preview and what they're typing will show up immediately (i.e. it'll be a native HTML form or ckeditor5). So 100 or 200ms server round trips will be considerably less of an issue - especially if we avoid spinners and jank in general, and make sure that replacing the content in the preview doesn't interfere with page interaction. So there is the potential to create a very smooth experience even with the round trips.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Thanks, that's very helpful context! And an interesting perspective that had not yet crossed my mind ๐ค
- First commit to issue fork.
- Status changed to Active
5 months ago 6:42am 23 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Evolve component instance edit form to become simpler: generate a Field Widget directly Fixed landed last night, which makes it easier to work on this :) I see @bnjmnm pushed a first WIP commit to the issue fork already โ yay, glad to see this moving ๐
- Status changed to Needs review
5 months ago 5:46pm 24 July 2024 - Issue was unassigned.
- Status changed to Needs work
5 months ago 2:55pm 25 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Detailed review on the MR ๐
I could see this being sufficient for now, to meet ๐ฑ Milestone 0.1.0: Experience Builder Demo Active goals.
But I don't see yet how this can fulfill all needs, so at minimum, this AFAICT needs follow-ups. How many follow-ups probably depends on what I missed aka what you can provide a solid answer to :)
Finally, some documentation, even if it's sparse, would help grok this and clarify next steps โ
InputBehaviors
should explain what assumptions it makes WRT widgets'name
attributes matching the Field Type property structure exactly, those values not needing transformations, how/if either of those could evolve, etc. - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Oh, and a screenshot of this in action, please ๐
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
In other words: what about field types with multiple props, such as ImageItem?
That would of course need to happen, and I'll make it more explicit in the code. It will be much easier to implement when the all-props component can be added as a page component in ๐ Create a component for testing form backend + frontend integration Active (or whatever issue is blocking that one).
I'm not sure if the solution will be a more clever parsing of the Drupal selector or if we figure out server side and provide that info in an additional attribute. I feel like what is here is worth adding now and addressing the different prop shapes in a separate issue - but if thats concerning it's NBD to do it all here and postpone this on #3463583. - Status changed to RTBC
5 months ago 7:13am 26 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
It will be much easier to implement when [โฆ]
๐ฏ โ plus, then there should be a broader spectrum of "widget/form complexity" โ because
StringItem
's widget is on one extreme, andImageItem
's is pretty much on the other extreme ๐Thanks for creating ๐ฑ [META] Redux sync on ALL prop types, not just ones with a single [value] property Active , that helps clarify the intentionally-out-of-scope parts of this issue. I just updated the title to make it clear what the difference is between this issue and its follow-up ๐
That would of course need to happen, and I'll make it more explicit in the code.
๐
but if thats concerning it's NBD to do it all here and postpone this on #3463583
Hell no! ๐ We want to iterate in small pieces, and now that there is a follow-up and you'll make the code/docs more explicit, I have no hesitation to land this ๐ Especially because it will surely help discover additional next steps on the front end.
I'm not sure if the solution will be a more clever parsing of the Drupal selector or if we figure out server side and provide that info in an additional attribute.
Interesting! I think that could totally work for most field types. ๐
Given only docs (including a
@todo <follow-up issue URL>
comment) and a screenshot are missing, and some minor remarks on the MR, I pre-emptively approved the MR, so you can address those bits and merge this MR, to end your week on an epic note ๐ - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
FYI: this will also unblock โจ Partial preview updates: update preview of modified component only, not entire component tree Active โ but unlike ๐ฑ [META] Redux sync on ALL prop types, not just ones with a single [value] property Active , that has no additional blockers, so it will become immediately unblocked upon this landing.
-
bnjmnm โ
committed cc383034 on 0.x
Issue #3462441 by bnjmnm, Wim Leers, jessebaker: Contextual form values...
-
bnjmnm โ
committed cc383034 on 0.x
- Status changed to Fixed
5 months ago 2:28pm 26 July 2024 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
Good feedback, all addressed. This is going in.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Congrats!
Unpostponed โจ Include component props form in undo Needs review and โจ Partial preview updates: update preview of modified component only, not entire component tree Active .
Thanks for that epic screenshot ๐คฉ
Automatically closed - issue fixed for 2 weeks with no activity.