Contextual form values need to be integrated with Redux: start with single-property field types

Created on 18 July 2024, 3 months ago
Updated 9 August 2024, 2 months ago

Overview

Redux integration - editing props should update the layout. This will eventually enable โœจ Partial preview updates: update preview of modified component only, not entire component tree Active which will optimize the layout updates and โœจ Include component props form in undo Needs review so the form inputs respond to undo.

User interface changes

โœจ Feature request
Status

Fixed

Component

Page builder

Created by

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ง๐Ÿ‡ช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:

    1. 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
    2. 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 3 months ago
  • ๐Ÿ‡ง๐Ÿ‡ช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 ๐Ÿ‘

  • Merge request !114#3462441: Contextual form -> redux Model โ†’ (Merged) created by bnjmnm
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • Issue was unassigned.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ง๐Ÿ‡ช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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • Status changed to RTBC 3 months ago
  • ๐Ÿ‡ง๐Ÿ‡ช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, and ImageItem'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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

  • Pipeline finished with Skipped
    3 months ago
    #234895
  • Pipeline finished with Skipped
    3 months ago
    #234897
    • bnjmnm โ†’ committed cc383034 on 0.x
      Issue #3462441 by bnjmnm, Wim Leers, jessebaker: Contextual form values...
  • Status changed to Fixed 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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.

Production build 0.71.5 2024