🇫🇮Finland @lauriii

griffynh → credited lauriii → .

This is likely needed by 🌱 [META] Experience Builder Personalization Active but otherwise I don't think there's a reason we'd block a release on this.

It looks like in the case of a required image for a SDC, after removing the image, the component fails to render due to an error. Is this expected?

I don't think this is a beta or stable blocker but would be a good to prioritize this with other non-blocking bugs.

restrict scope of this issue to only those prop shapes where #1 (a client-side transforms) exists, and which meets #2 (transforms to a valid resolved value per the JSON Schema for that prop)

Where can I find a list of prop types that meet this criteria?

I'm running this into a situation where I don't even have the override enabled. Debugged this for couple minutes and I can confirm this isn't caused by template_preprocess_breadcrumb__as_js_component(); the problem is lower level than that. It seems likely that this is caused by the fact that we are not using the regular routes for rendering the preview, but \Drupal\Core\Breadcrumb\BreadcrumbManager::build uses the route match as the basis of rendering the breadcrumb.

Realized this is a duplicate to 🐛 Breadcrumb is not rendering with the right context Active .

lauriii → created an issue.

I can't reproduce this anymore. Back in October we had a myriad of media related bugs so this could have been fixed by one of those.

Decreasing the priority now that 📌 Always create a new revision on publish Active has been committed.

This was fixed by ✨ Refactor Layers UI to use DnDKit Active .

lauriii → created an issue.

One example use case we should think about is lists. If you need a list component that can include links, text, and images, how would we build that?

I like the idea of dynamic slots but AFAIK neither Twig or React really supports this. #5 seems like it could potentially work for XB but wouldn't really work outside of XB. Or can we think of a syntax that would allow us for other Twig template to pass multiple slot values?

I don't think this issue at least justifies parsing Twig markup. It seems if anything, we should make the client a bit more resilient.

It would be nice to have client side validation for this still for UX purposes. Reducing priority.

Yep, it looks like this has changed shaped since this was first filed. This is not super critical though so moving to minor.

I think we should express the uploaded videos as follows in schema.json:

{
  "video": {
    "title": "video",
    "type": "object",
    "required": ["src"],
    "properties": {
      "src": {
        "title": "Video URL",
        "type": "string",
        "format": "uri"
      },
      "poster": {
        "title": "Poster image URL",
        "$ref": "json-schema-definitions://experience_builder.module/image-uri"
      },
      "width": {
        "title": "Video width",
        "type": "integer"
      },
      "height": {
        "title": "Video height",
        "type": "integer"
      },
      "autoplay": {
        "title": "Autoplay",
        "type": "boolean"
      },
      "loop": {
        "title": "Loop",
        "type": "boolean"
      },
      "muted": {
        "title": "Muted",
        "type": "boolean"
      },
      "controls": {
        "title": "Show controls",
        "type": "boolean"
      }
    }
  }
}

For oEmbed, I'm not sure we need the max width / max height here. It seems something that the component itself should handle? We may need to expose the height / width of the resource, as well as title which would make this shape as follows in schema.json:

{
  "oembed_video": {
    "title": "oembed_video",
    "type": "object",
    "required": ["url"],
    "properties": {
      "url": {
        "title": "oEmbed URL",
        "type": "string",
        "format": "uri",
        "contentMediaType": "application/oembed+json",
        "x-oembed-type": "video"
      },
      "width": {
        "title": "Video width",
        "type": "integer"
      },
      "height": {
        "title": "Video height",
        "type": "integer"
      },
      "title": {
        "title": "Video title",
        "type": "string"
      }
    }
  }
}

For documents, to display a link, we'd need a bit more than just the URI in the schema.json:

{
  "document": {
    "title": "document",
    "type": "object",
    "required": ["src"],
    "properties": {
      "src": {
        "title": "Document URL",
        "$ref": "json-schema-definitions://experience_builder.module/document-uri"
      },
      "title": {
        "title": "Document title",
        "type": "string"
      },
      "description": {
        "title": "Document description",
        "type": "string"
      },
      "filename": {
        "title": "Filename",
        "type": "string"
      },
      "filesize": {
        "title": "File size",
        "type": "integer",
        "description": "File size in bytes"
      },
      "mimetype": {
        "title": "MIME type",
        "type": "string"
      }
    },
    "document-uri": {
      "title": "Document URL",
      "type": "string",
      "format": "uri-reference",
      "pattern": "^(/|https?://)?.*\\.(txt|rtf|doc|docx|ppt|pptx|xls|xlsx|pdf|odf|odg|odp|ods|odt|fodt|fods|fodp|fodg|key|numbers|pages)(\\?.*)?(#.*)?$"
    }
  },
}

Should there be a reverse relationship too? An example of this would be to limit people to put accordion_items only into accordion_group components.

I was thinking about this but I'm not sure that we can do that because components can include multiple slots and we don't have the concept of a default slot. We don't know which slots these restrictions would apply to in the case that a component includes multiple slots. I would recommend opening a follow-up issue to consider this after we've added slot restrictions in this issue. This seems something that could be easily added as a new API there.

penyaskito → credited lauriii → .

I think the main drawback of one field per exposed slot is with respect to future Views integration. Suppose in the future we want to be able to support Views (or even just EntityQuer(ies)) for finding all instance of a Heading component whose element prop is equal to h2 (or likewise get lists of component instances matching whatever other condition). If we want such a View to be able to query across all exposed slots, I think that gets more complicated (maybe not impossible) when the data for each exposed slot is in a different field.

There might be other use cases that prevent us from going with the field per slot approach but this tradeoff seems acceptable to me. I can't think of use cases where we'd need this.

The slot itself is supposed to convey something and we shouldn't have to be able to take components outside of that context. This is only relevant in the case of a single exposed slot, e.g.,

content

where the slot is less relevant. But when you have two slots, there's already meaning attached to it, e.g., content_top and content_bottom.

To continue with the heading example, a module helping user to create a proper heading tree, may want to list all of the components inside all of the slots, and filter them based on whether that component is adding a heading. Even in this case, I'd imagine this would happen always in the context of the slot. It wouldn't be all that useful to show that there's a h2 somewhere, you'd want to show that there's a h2 in the content_top slot.

Also, wouldn't Paragraphs have a similar limitation to this, when you use multiple entity reference fields (i.e. multiple slots)?

Debugged this a little bit and this seems related to 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed .

Looks like this is potentially also impacting 🐛 Create New Revision checkbox is always unchecked and doesn't create revision Active .

There could be more than one problem here. I was able to get the boolean accounting to work but regardless of that this is not working. The value is missing when $items->getValue() gets called on that field because \Drupal\Core\TypedData\Plugin\DataType\Map::getValue ignores the pathauto property because per \Drupal\pathauto\PathautoItem it's a computed field.

✨ Refactor Layers UI to use DnDKit Active has landed.

This was solved by ✨ Refactor Layers UI to use DnDKit Active .

Indicating that this could be worked on by anyone.

I don't think we should block the release on this but we should try to get this fixed regardless.

I've been thinking how we should name this. I'm thinking supports and conflicts would be nice because it would make it explicit that we're documenting what components work nicely in the slot. This is relevant meta information even outside of page builders because it would allow developers to document what set of components should be tested with the component.

I think we need to support minimum and maximum children too. Often times there's a limit to how many components a slot is able to accommodate.

Allowing the use of categories and tags allows building design systems that are flexible without introducing a high number of direct dependencies between components. Adding tags besides categories allows managing situations where there's tension between categories which would be used for categorizing components in the UI and the restrictions.

Some example uses of tags could be:

1. full_width
2. editorial_content
3. dark and light

This way one could configure that XB root only supports full_width components. If you have components that specify a dark or light background, you could specify that the component only supports dark or light components.

I'm thinking that the following schema would be able to satisfy all of the use cases I have in mind:

slots:
  slot_name:
    title: string
    minimum: integer
    maximum: integer
    supports:
      components: array<string>
      categories: array<string>
      tags: array<string>
    conflicts:
      components: array<string>
      categories: array<string>
      tags: array<string>
    examples: array<object>

Here are some example configuration which would support components tagged as "feature" to be added. There could be anywhere between 1 and 6 items. Example values allow specifying content that a page builder could add to the slot by default:

name: Feature Section
slots:
  features:
    title: Feature Items
    minimum: 1
    maximum: 6
    supports:
      tags: [feature]
    examples:
      - component: "my_theme:feature_card"
        props:
          title: "Fast Performance"
          description: "Lightning fast load times"
          icon: "speed"
      - component: "my_theme:feature_card" 
        props:
          title: "Secure & Reliable"
          description: "Enterprise-grade security"
          icon: "shield"
      - component: "my_theme:feature_highlight"
        props:
          title: "24/7 Support"
          badge: "Premium"
          link: "/support"

Adding steps to reproduce

People administering XB would be the ones deciding what behavior they want for what gets done with the examples. For example, someone using XB to create a demo site for a DrupalCon presentation might want examples to be the initial value, and stored with the content, as per the current behavior. But someone making a corporate site for production might want SDCs' example values to help give some guidance to the content creators, but not get accidentally leaked to the published site.

It's unclear to me still how we'd implement this at this point. I think we have to design the no-code component editor before we make decisions around implementing this. There's tension between the single directory nature of SDCs and making decisions like this in XB UI. However, if we can allow users to achieve all of this by easily decorating a component via the UI, it wouldn't actually be conceptually modifying the component but creating effectively a new component.

I think we'll want to implement something more explicit like wrapping/extending the widget to give the content editor a choice to either fill in the decorated/base widget or choose a "Use the default" option, or something like that.

💯 👏

The components load really fast but I definitely still see the loading animation briefly even if I wait for bunch of time before:

The components load really fast but I definitely still see the loading animation briefly even if I wait for bunch of time before:

Yep, that's what I'm referring. Same pattern would apply in future for Sections and no-code components.

+1 that it's pretty annoying that clicking the component adds them to a page.

Could we use the clicking of a component to open the component in the code editor? It's pretty annoying to have to right click components to edit them. I realize the + button to indicate opening the library doesn't necessarily make sense with this so we may want to get some opinions from the design team.

I see you added loading states to the different component categories. I'm thinking it would probably make sense to load at least components on the initial load to avoid users seeing the loading animation.

We should definitely target this for 1.0 but we can reconsider in a later triage more closely.

I don't think this needs to be postponed. This should work regardless of the States API not working.

Spent few minutes debugging this to make sure this isn't a duplicate and I agree.

It looks like the problem here is that \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields has special handling for boolean fields. However, that handling does not account to the fact that fields can have multiple properties, and therefore other field types could include a boolean field. This is why the path field is not being detected for having a boolean, which is added by \Drupal\pathauto\PathautoWidget::formElement.

This problem seems pretty fundamental because you could run into this with other field types too. The simplest example would be Field Union fields. Updating issue summary based on this.

lauriii → created an issue.

In one of the early conversations with @lauriii he was very concerned that the data model should reflect how things appear on the page (e.g. ordering not getting out of sync). I get the principle that only ordering within a slot would actually affect the rendering though.

This is probably one of the conversations at dev days but I have no recollection whatsoever on what the specific concern was. What's important is that components within a slot are ordered correctly. I don't have strong opinions on whether that's done with field delta or not.

What's also important is that there's a simple way to construct the tree of components because we may have decoupled clients like Lupus building alternate rendering engines for the data model.

Amazing work on this @jessebaker 👏

Let's make sure to test that the UI handles this gracefully to not introduce more error handling debt.

+1 that we should not stick to the current page title block if it turns out to be more work than working around it would be.

it's restricted to the Page content entity type. But e.g. article nodes' path alias should have this same UX?

This is currently restricted to Pages (since that's all that XB supports). We could expand this to Nodes in future once we work on those.

If the answer to the above is that we shouldn't make it work for component instances, then … why don't we just create a new widget and at-runtime override the entity form display to not use \Drupal\path\Plugin\Field\FieldWidget\PathWidget?

I don't think we would make this work with component instances since this is specific to the path field type which is tightly coupled with aliases. Not creating a new widget makes adding Pathauto support simpler most likely because Pathauto is extending the default widget. If that's not the case, it seems this would be really easy to refactor to a widget if we want to do that as a follow-up to 🐛 XB pages doesn't respect Pathauto widget Active .

Addressed feedback on the MR

This seems potentially duplicate of 🐛 XB pages doesn't respect Pathauto widget Active or maybe there are two separate problems?

lauriii → created an issue.

@daffie Thank you for the detailed analysis! I totally agree that performance is critical for Experience Builder. Improving performance over the existing solutions was one of the goals since the beginning.

Are you aware of the discussion in 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed ? The issue includes most of the intended changes. In #3468272-33: Store the ComponentTreeStructure field property one row per component instance → there was an assessment from @catch where he stated that the expected performance impact would be largely neutral. It would be great if you could look at that issue and provide any further insights there!

What comes to relying on PostgreSQL, the challenge is that if we introduce changes to the hosting requirements, it will slow down adoption. Because of that, it seems unlikely that Experience Builder would introduce a dependency on PostgreSQL.

Could we place these after code? Then these are in the order of how frequently they are going to be accessed. It's also more logical because Components + Code are related, and Dynamic Components + Overrides are related.

There's some more work needed to get pathauto to fully work with XB: 🐛 XB pages doesn't respect Pathauto widget Active . We need to do that issue regardless of what we do here.

I think we should do the following for the pathauto integration in the other issue:

1. Make pathauto not break the path field when there's no pattern configured for the entity type
2. Make sure that the solutions do not conflict, i.e. this should be only loaded when pathauto hasn't been enabled

I'd be more than happy to tackle 2. but I think we need to fix 1. more urgently first.

+1 for type: string, format: uri-reference not being relevant here.

penyaskito → credited lauriii → .

penyaskito → credited lauriii → .

penyaskito → credited lauriii → .

+1 for this being a beta blocker because of data loss.

Yep, that's correct! 😎

I don't think we would do 2. 1 seems something we might do but it also seems like a new API to specify attributes outside of the schema. Same as how https://github.com/rjsf-team/react-jsonschema-form separates UI Schema from the Schema.

balintbrews → credited lauriii → .

Unpostponing since in case that folks want to contribute to this.

Would you be able to workaround this by adding a prop that controls whether the details element is collapsed or expanded?

#19 🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active : Exactly. Providing an API for this would help improve the UX/DX since it would allow users to see the default value on the form – but I don't think this is critical. The reason this gets surfaced as a critical feature from Site Studio users is because there's literally no way to achieve this there.

I would imagine if we did this, we would still keep the examples[0] behavior just as it's today. Then we'd add support for default which would have a different behavior.

Sorry about that. I think I got confused because it looks like #12 🐛 Setting default values does work for boolean props Active was tested with code components but the original issue was for SDC 😅

So let's remove that restriction from both patterns and field component tree 👍

#17 🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active : I understand clearly that there's a limitation where the current example values are stored in the database. If you don't want that behavior, you can define fallback values in the SDC/React component. I understand we may want to provide an API for this in future for an improved DX+UX but I don't see how that should be critical and block a release.

Best experience would be to support adding the title block for pages too. How difficult would this be? If this is something that would be difficult to build and/or hard to maintain, we may want to reconsider that.

@Wim Leers When using asymmetric translation (meaning that both layout and content are translated), wouldn't the label always be translated for different languages? What I meant is that there shouldn't have to be any specific handling for these labels since if everything is already translatable in that language, so should the label be. I realize that comment was naive because we're now changing the data model and this may not be true anymore with the new data model.

Tested 📌 Handle data fetching errors in PageInfo component Active and looks like the fix there is great except the error message is now visible as a message in the preview.

lauriii → created an issue.

lauriii → created an issue.

I'm proposing to add a new widget for generated URL Aliases in ✨ Creating a page generates the URL path dynamically when editing. Active for better authoring UX. I wonder if this validation could be added on top of that?

lauriii → created an issue.

Converted the JavaScript code to a React component and implemented it. Updated the issue summary to match the implementation.

lauriii → made their first commit to this issue’s fork.

Adding a related issue here

lauriii → created an issue.

🤔 Not sure what to do here. I bet we could try to figure out a way to render the XB component tree inside the node.html.twig template (writing this on phone, can't quickly read node.html.twig to make sure). But … that kinda goes against the spirit/point of XB.

Nodes should be rendered using node.html.twig so long as Content Templates are not involved. When a Content Template is defined for Content Type, it would essentially take over the rendering and no longer use node.html.twig. This shouldn't be a massive problem because unless you have defined a Content Template, there aren't any slots to edit in XB.

Find out any possible downsides - Alex mentioned that some components won't require all their props to be translatable. This model will allow us to do that on a per component basis as we're one row per instance.

These seem potential benefits of this approach rather than downsides 🤔 Have we considered what are the downsides with this approach? How does this impact performance, flexibility, storage requirements, and developer experience?

I'm also curious, how does having one row per instance help with prop translation? Might be worth explaining the change in more detail in the issue summary because I feel like I'm missing something.

This is different in Site Studio where there's no way of setting defaults/fallbacks for empty values AFAIK? In Experience Builder, both SDC and in-browser code components provide a way to use fallback values in code.

UI Suite themes are using this approach too. See https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/1.1.x/components... for an example. This is also what their docs are recommending: https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... → .

To me this seems largely a docs concern to explain how we'd recommend components to be built. There are potential UX implications because many page builders have special handling for empty behavior where they don't even show the field when the fallback value is used.

Crucially, #8's when you see prefilled text, you expect it to be submitted unless you overwrite it. ignores that it cannot work like that for SDC's default images — otherwise they'd end up being saved into Drupal's Media Library.

This seems just the incorrect behavior of the widget itself? We should make it possible for the widget to display the default value, regardless of it not being in Media Library. The widget is just means to displaying the value of the field, which in Drupal has historically been tied to a specific field type. In this case it's interesting because the example value cannot use media library and therefore we must implement a field widget which is able to handle display of multiple field types.

📌 [UI update] Lock left and right panels to the sides of the screen so they no longer render "over" the canvas. Active has been committed.

I don't think this is either a beta or a stable blocker. 👍 We could potentially remove later phase in case someone wants to contribute to this since it would be a nice UX/DX improvement.

wim leers → credited lauriii → .

This won't be needed anymore after: ✨ Move zoom controls, show only one viewport Active .

The fallback rendering actually was rendering the subtrees on both the live site and in the preview. That's what the Fallback source was literally doing.

Is this still the behavior? If it is, we need at least a follow-up to revisit that.

This is not a fair comparison IMHO. If an SDC/code component adds a new required prop, the component will refuse/fail to render if it is not populated. Content entities in Drupal that don't have a required field just won't render that field, and subsequent edits will require a value to be specified.

The point was that core is not protecting the data from becoming invalid; instead it deals with it on the runtime. This is probably something we will have to do as well when it comes to changing props.

FWIW, you will run into that exact same problem if you use SDCs for rendering the content. Because of this, using required isn't common for components. For example, UI Suite themes are not using required fields at all.

So, you're proposing a new issue for identifying and surfacing dangling/dead component subtrees?

Yep, +1 for a follow-up on that. And +1 for postponing that until we have made decisions around what the data model is going to be.