@e0ipso

I want to hear @pdureau's take on this. Let's see if I can lure him in to add his thoughts before we decide on anything.

tim.plunkett → credited e0ipso → .

This summary above is part of my conversations with @penyaskito on the topic. Please, consider granting credit to him.

I believe BC will complicate things here, now that SDC is stable. But I am getting ahead of myself.

I think we have to:

Define how to call the variant from Twig

This is the template ID. If we want to keep ourselves to native include / embed / extend we need to create a new naming convention. Potential ideas:

include('<provider>:<component-name>#<variant>', ...)
include('<provider>:<component-name>.<variant>', ...)
include('<provider>:<component-name>--<variant>', ...)
include('<provider>:<component-name>|<variant>', ...)

Define the naming convention for the template file name

Here we should probably mirror the decision above.

<provider>/components/<component-name>/<component-name>--<variant>.twig
<provider>/components/<component-name>/<component-name>.<variant>.twig
<provider>/components/<component-name>/<component-name>|<variant>.twig

Connect the template ID with the filename

The ComponentLoader will receive a template ID, and will be in charge of finding the template file name, as defined above.

For the render array way of embedding things, the ComponentElement will take a new optional #variant (as proposed in the IS), and will compose the necessary template ID. Everything else is the same as using Twig to embed the component.

The component plugin Drupal\Core\Plugin\Component

At this moment we access the template for the component via a public property $component->template. However now the template to be used for the component will depend on the variant. We need to introduce $component->getTemplate($variant) (which could throw a new MissingComponentVariantException). This is a problem for backwards compatibility.

Proposals to tackle this issue should include a plan for backwards compatibility.

marcoscano → credited e0ipso → .

Embedding nested components is done the same way you would do it in a regular twig template. Perhaps this example can help: https://git.drupalcode.org/project/sdc_examples/-/blob/1.x/components/si...

Thanks @mortona2k and @freelock! Please, ping me in Slack when you want my attention back to this issue.

This may also be true for field mappings correct? I mean, rendering a field that contains an integer to pass it to a component will yield "3" and not 3 as we want.

Updating the Issue Summary to reflect this.

I like this idea!

Wonderful!

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

This should be fixed now.

I think that before we can move forward with this, we need to clarify what will be the role of Experience Builder in this space. I cannot imagine a first-in-class page building experience that doesn't implement all of those features.

My take is that it is seldom useful to contribute a single component. I am sure the use case exists, but it will not be the norm for sure. I think that a "bundle" of components (a school of components? a gaggle of components?) will be the norm. Like in other technologies.

I agree that this feature is very useful. We thought about it a bit when designing SDC. Our idea was that a this bundle should be a module. You can have a module that only contains components. Components are discoverable within a module, and have rich metadata (like thumbnails, README, schemas, descriptions, etc). It is not too far fetched to imagine that you can search for an accordion component in project browser, and you find the Simple Components Extras Plus module.

Using modules has certain key advantages:

• They already have documentation on how to author them, how to download them, how to enable them.
• They already have the technical tools to author them, download them, enable them.
• They have the concept of versioning built in.
• They have the concept of a dependency chain built in.
• A component cannot currently exist in isolation since you put them in the page by: my-module-or-theme:my-component
• Modules can also house dependencies for the components, like new Twig filters.
• Modules can also contain tests. And our CI tools also know how to run those tests.
• We have tools to upgrade modules (and the components within)
• ...

This seems to go inline with some of the comments in this issue.

Thanks for helping move the conversation forward @dafeder!

I am adding a note to the IS to note that Composer also depends on this package. This might add some additional confidence in adding this as a runtime dependency for Drupal.

🤣 this made me laugh @mherchel!

Yes, I am interested in being added as a maintainer.

Pierre, like I already told you, thanks for being part of this community, and for putting up with me 😜






e0ipso → created an issue.

I think that the field group should appear in the mapping options (as stated in the IS), and the fields within that fieldgroup should not. Thoughts?

👏👏👏

I am able to reproduce this.

I see some of the stories are plain text for the slot content. I would love to see them with HTML markup to ensure they are not improperly escaped.

penyaskito → credited e0ipso → .

Another potential example to look at is the Carbon Design System, which is Open Source.

Is the intent to have a list of components we may want to support? Or are we looking for an actual set of components? If we are, we will likely need the design system for them (or we can extrapolate it if we are building the components for an existing theme like Olivero).

However, there is a chance that what we are looking for theme independent components. If that is the case, we might want to discuss first how to best implement those.

I wonder if we could solve this by just adding use DependencySerializationTrait; to the class. Any chance you can try that?

Ah! Thanks for the clarification @Wim Leers. I was taking it as a "there is no need to dwell too much in this now, since we'll be able so solve it with a build step".

To pile on that spirit that @effulgentsia brings on #33, I'll say that using industry standards will bring us more and more of these tools for free. Not only to generate the schemas, but also to read and leverage them. Staying true to the JSON-Schema standard will also allow us to auto-generate forms for the component props, synthetic examples, etc. Carving out (more) exceptions like drupal-image will make these tools fail without specific handling.

While I am an advocate of TS (I even pushed an ADR at Lullabot to the effect), I do not think that we should prescribe it in order to write re-usable schemas. This is specially true since we are not writing JSX components in Drupal (yet).

In any case the import method is a bit similar. In one case we defer to the native module loader and a URI, and in the other we use a custom code loader and a URI.

Tagging for summary (and title) update based on #6 and #7.

Probably only tangentially related, and off topic. But my mid-term goal would be to replace render arrays (and maybe Form API) with components as well.

Without having dug too much into this yet, my question is: is this specific of SDC adoption? Or is it also a problem where templates for entities (et al) are not thin wrappers for the render array?

In other words, would it have been a problem as well with the older version of the template you linked in the IS as an example?

(added Component Blocks to the IS)

There is wrong information in the MR in some sections. There is also incomplete information in others, and confusing information elsewhere. Please take a look at the project page, I took the time to write that info and to review this MR.

Yeah. I even have a slight preference with $ref: module-components://experience_builder#definitions/image* as mentioned in #27. Not because it is easier to write, or easier to remember, but because it's easier to copy&tweak and easier to document (specially when the component author is already writing optional JSON-Schema).

*My slight preference comes from the fact that it uses the $ref recommended syntax JSON Pointer. Which is documented for us, and the resolution mechanism is already implemented in the justinrainbow/json-schema library without additional stream wrappers.

Creating components is a common enough task that it should be really easy for someone who is a) not familiar with Drupal b) not a senior developer and hence c) does not know JSON-Schema.

I think that from the SDC perspective:

a) Writing a component does not require much knowledge about Drupal, by design (as you very well know from being a co-conspirator on SDC)
b) I don't think this is required for writing SDC, or referencing schemas.
c) I think this is a soft requirement. If you want to write components with schemas, you kinda need to know they way they are written (JSON Schema). Now, do you need to be an expert? Definitely not.

I think the discussion is not weather or not it's OK to elevate the barrier of entry to newcomers to Drupal, I think we all agree on keeping it the lowest possible.

I think the discussion is that @lauriii in #29 argues that the $ref thing is complicated, and might put people off. And I think that using made up types like drupal-image makes things more complicated and dis-empowers said newcomers.

In #27 I tried to argue why an open list of made up types is bad UX. How do we document the made up types that contrib modules provide? How do we make the eventual documentation page discoverable for front-end developers? How do we tell newcomers that there may be other types? (in addition to all those challenges, now we are back to our island instead of following an industry standard... I think I am repeating myself at this point, sorry).

That's tricky because I disagree with the premise in #25.

I think type: 'drupal-image' is way more confusing. It only gives you a label. It doesn't give you any info about what a drupal-image is, what are its dependencies, or were to find more info about it. Everything is implicit with that.

On the other hand, using $ref: <uri>#<locator> you are:

• Using a standard and well documented feature of JSON-Schema.
• Using another standard (JSON Path, in this case) to locate a data sub-structure within a file.
• Declaring that this depends on the experience_builder module.
• Giving the user a clear place to look for the definition and investigate further.
• Using the same industry standard pattern already in use in other frameworks leveraging JSON-Schema, like OpenAPI definitions.

I agree with @pdureau that perhaps a different URI template would give less pause. $ref: module-components://experience_builder#definitions/image would be my preference, but I realize that's exactly what Lauri is trying to avoid.

This is great progress! It shows that it can be done.

thanks to a new ./schema.json file in my module named experience_builder, [...]

I would advocate for a name that is more specific to components. I propose components/shared-schemas.json. This is because it is scoped to the components/ folder, and it conveys that you only need this if you want to share definitions.

plus a new stream wrapper service inspired by @pdureau's superb work in ui_patterns 2.0.x (see #17 — I'd never have found it otherwise!).

justinrainbow/json-schema has a the concept of UriRetriever, which accomplishes the same. The advantage is that by following their pattern we can resolve schemas recursively every time (without having to pluck out the reference and resolve it manually). This includes loading the schema for validation during development time.

Merged and released. Something went sour with the commit credit :-(

I think this looks good!

One goal that I want to keep in mind is that this needs to have a very low friction. That's why I initially thought about letting components reference props in other components. This had the benefit of not having to do anything to declare the referenceable schemas. Sadly, this is not viable. Mainly because the referenced component may be deleted, and then the schema would break. This stems from the fact that there is no dependency chain between components (which I believe it's a good thing).

So, if we don't want to introduce dependencies between components but we want to re-use schemas, what dependencies do we introduce? I think the easiest is to depend on modules / themes. They are the things we install and uninstall, and we do check dependencies on such operations.

#7 proposes a schema-definitions.json in a module, which can be referenced as module:/modulename/path-to-any-file#json-path-to-schema-definition. This may be too lose, and therefore give pause and be hard to document. Perhaps we want to enforce a specific name for the file (how about components/prop-types.json?), and then reference it component-schemas:/modulename#json-path-to-schema-definition.

e0ipso → created an issue.

Adding a reference to the sponsor's logo.

enforce_prop_schemas is only used to make sure components in the theme contain a valid schema. It is not meant as an on/off switch for schema validation.

As I see it, if you don't want schema validation, there are two solutions:

1. Delete the prop schemas, and set enforce_prop_schemas: false
2. Keep the invalid schemas, and turn off PHP assertions.

@agentrickard any chance you can write a note in the SDC documentation on this topic → to avoid other tripping over this? Right now docs are a bit bare bones and could use some love. See screenshot below.

This has come up in the past in Slack. See this thread. Screenshooting for convenience:

In that thread @pdureau mentions:

Prop default values are not automatically injected into the template and that's a good thing IMHO. They can be used by the SDC ecosystem to build admin pages forms, where they will be the form element default value. You can set a default value in the template with the default() filter.

I tend to agree. The JSON-Schema specification allows the default keyword, and we support JSON-Schema. However, I don't think the intention for it should be to inject data into the template. I think the correct use for it is to inform the application of a default value when building forms, or generating synthetic example renderings of a component, etc.

While we could do what the IS seeks, we would face significant challenges down the road. Imagine when we need to translate the default value, or if the default value is a site URL and the site is installed in a subdirectory, or the default date should follow the site configured time zone, ...

In general, imagine that we need to use dynamically processed data as the default (which I think is a common use case). For that we need a place to declare the default with access to a runtime. The current recommendation is to use Twig to do so. This makes sense for me because:

1. It is the place where the data is provided to the component. That be via mapping fields from a content type, providing a hard-coded value, ... or doing any of those but with a defauls. Ex: content.field_foo|default('my default'|t).
2. It is a place that can access the necessary APIs for those dynamic behaviors mentioned above.
3. It is a common established practice in Twig that front-end developers already know.

However, I acknowledge that there is confusion around this. The fact that it has come up more than once is an indicator. I wonder if adding a section in the official FAQs is the way to fix this.

From what I understand, while the module is in experimental phase, all the classes that could/should be provided by other modules like ShortcutsNavigationBlock should stay in Navigation module. They're moved when the module becomes a first class passenger.

I forgot to mention. If you are planning to move classes, take a moment to find the correct eventual location. Once done, add class_alias to the current location.

It's stable now!

Apologies for chiming in without any context on the module (yet). I was pinged for ideas, and here I am.

From past experiences I would:

• Define the back-end APIs of the experimental module. Anything that is not an explicit API should be impossible to use as such. Mark dockblocks with @internal, make services private, turn classes or methods into final, make methods private. This will help with making it beta (more on that below).
• Perfect is the enemy of good enough. There is no innovation when aiming for perfection. Get it into core. This means that if there are any lingering issues that tend to get into bikeshedding ask yourself: is this remaining thing critical enough to miss the deadline for 10.3?
• Remember that experimental modules are removed from the codebase before a tag is made. This means that there is a chance that you put this into 10.3.x and you don't see it in 10.3.0, 10.3.1, ... until the experimental module is marked as beta. Create an issue to mark the module beta. You can use 📌 Mark SDC as beta so it can be included in 10.1 Fixed as a reference.
• Start the official documentation. Even if it is just a big TBD. This will give you a place to link things to. This is the SDC one for reference: https://www.drupal.org/docs/develop/theming-drupal/using-single-director... →
• Define the front-end API. I am ignorant on how this works, but I think you should give this some thinking. It is worth showing that you put some thought into it. Things like: What is the markup that themes can rely upon not to change? How will they alter/extend the look & feel? Using SDC would solve this because anything internal to a component is subject to change, the API is the the *.component.yml file, and you alter/extend with the usual component replacement.
• Framework managers, and core committers are the busiest people in the community. Walk the fine line of reminding them that this needs attention, but without pestering (they may have more pressing matters to attend to). They are gifting reviews, try to have everything ready for them before you summon their attention to the MR, as an expression of gratitude for their time.
• On my part, I can commit to a review for the back-end part of the module, once the PR is up. This should make it closer to an RTBC (necessary to get any eyes on this). Plan on having someone to do the review for the front-end part, do not wait for spontaneous reviews (those may not happen). If you don't find a reviewer, try trading reviews with someone.

Adding credit. Thanks both!

We are also using this and it's working for us.

Update docs after 10.3 changes.

I assume you want to put a block in a block because you want to nest components. That means including a component in a slot. Slots are free HTML. Editorially this means a textarea, which SDC Block enhances with a WYSIWYG. I think that the missing piece is how to add a component into an HTML textarea using the WYSIWYG.

Including the component in the display mode template.

Thanks for reporting back. I feel that we should aim to fix this issue, given that avoiding display mode templates is the main goal of this module.

I did another review path. I feel this is getting closer and closer, but not there yet.

I think the necessary follow ups after 🌱 [META] Move code from the experimental SDC module to core Active have all been addressed. They were all included in 📌 Remove SDC deprecated twig functions before 11.0.0 Postponed .

I am inclined to close this issue as well.

I think this one is no longer necessary. This was addressed as part of the work in 🌱 [META] Move code from the experimental SDC module to core Active .

📌 Document deprecation in drupal.org documentation pages Active and ✨ Mark sdc as obsolete for 11.x Active are both closed by this commit.

As far as I can see, this was completed in 📌 Remove SDC deprecated twig functions before 11.0.0 Postponed .

This was completed as part of 📌 Remove SDC deprecated twig functions before 11.0.0 Postponed .

Moving back to RTBC.

Resolved merge conflicts and fixed an issue number to point the the CR.

Review feedback implemented. Everything is back to green. Setting back to NR.

[déjà vu]

Oh no, I might have been looking at the wrong thing. I see an issue about deprecation messages. Investigating.

Review feedback implemented. Everything is back to green. Setting back to NR.

I agree with the main sentiment that the shortcut module has the potential to be very useful. I agree with these proposals. 👏👏👏

One of the challenges I see with this is that we will have to identify and manually handle all these non-printable keys. This includes bubbling, and or erasing some keys.

I would love to have this feature, but I am not sure a solid / maintainable implementation is feasible. Let's see what an initial patch looks like.

@freelock any chance you could take this through the finish line?

What happens if the extra field is empty? In that case we should not skip the static mapping, but with the current code we would be doing so.

I think the current solution you mention is the actual way to go.

Without this, I am creating an extra display mode to use SDC, and using the rendered entity formatter to show it. However, in one case I have a prop for an html tag, and it would be nice to configure this from the parent display.

After rendering, an entity can no longer be considered structured data. Therefore it needs to be put in a slot.

I suspect that the issue stems from the point proposed in #3.

How does this play out with https://www.drupal.org/project/sdc_block → ? I think of that as the main integration point for Layout Builder.

I think the another way is to ensure that your components always print the attributes.

Example:

<div{{ attributes }}>...</div>

Thanks for offering a solution @mortona2k!

@mortona2k did you solve this issue?

Added some questions about the schema URLs.

Yeah. I have addressed that. Sorry for the hasty change, I keep finding myself out of context for this issue every time I work on it. Thanks for catching it. Schema URLs are not super important but we got to get them right.

Pipeline is back to green.

I resolved merge conflicts and addressed @nod_'s request in #102.

+1 to @pcambra as well from me.

I co-maintain modules with him, I co-admin the Mastodon server with him (he does almost everything), and I have known him and his dedication for the project for about 12 years now. He is one of the people I look up to. I think we are lucky to have him involved.

I am a bit puzzled by this. This code should only execute when sending a response from the storybook.render_story controller. Which in this case is guaranteed to pass those if statements, right?

In any case, I am all in for defensive coding. So I will merge this one.

Thanks for catching this!!

Thanks for this!

Yes, I agree with @markie here. We should have a section (hidden under a details tag) that retains the previous docs. Something like Alternatively, you can also configure it manually:.

What do you think?

This looks good. Let's give it a go! Thanks!

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

Add schema for empty props.

@mortona2k that is a cool technique! I would love to know more about it. Would you be up to providing a short screencast explaining it? Or maybe even some explanatory screenshots? I know I am asking a lot from you lately, feel free to tell me so. I appreciate your involvement!

Hi again @mortona2k! It seems like yesterday we were chatting in Pittsburgh.

Component schema is mandatory for integration with SDC Display. The way to declare empty props is not by leaving them out but by actually saying they are empty. Perhaps a schema like this would work:

$schema: https://git.drupalcode.org/project/sdc/-/raw/1.x/src/metadata.schema.json
name: Info Card
props:
  type: object
  properties: {}
slots:
  card_title:
    title: Card title
  card_year:
    title: Card year
  content:
    title: Content

Thanks to this report, I have created an F.A.Q. entry in our documentation. I hope this fixes your issue. https://www.drupal.org/docs/develop/theming-drupal/using-single-director... →

I think this is an error message we should work on in core, so it doesn't blow up in contrib. Do you know if you had validation turned on in your local environment? If so, did it show any validation error based on the empty props declaration? I think we should have an issue for Drupal core to provide a good error message for this, to avoid tripping more people. Can you create this issue and shoot me the link in Slack?

Additionally, I think we also need an issue for https://github.com/e0ipso/schema-forms-php to provide a useful error message when validation is turned off for SDC in Drupal. Would you also create that one please? 🙌

Added new F.A.Q. entry for empty prop schema.

@catch you are correct, that is some cruft from the other day when I split that service in two. I will fix now.

Sorry @longwave it seems the suppression file had a syntax issue. It is fixed now.

@longwave I fixed the library bc layer and added a test for it.

Setting back to Needs Review.

Oh, would also be interested in your thoughts on service naming as well, because all other core services have friendly string names instead of just class names.

This is something suggested by @larowlan in the MR that got SDC in core as experimental.

Throwing something at the CI before logging off. I will finish applying the feedback in the morning.

@longwave thank you so much for your rounds of review. I know this is a big MR, with lots of context, and most of it is hidden behind children issues. I tried to organize it in sub-issues to facilitate development and review, it turns out it's been more difficult for everyone involved. Sorry.

The BC layer for the component validator service is incorrect.

You are correct, I will push a fix right away.

TwigExtension is getting quite large, should the component extension just be a separate service?

Yeah. We touched on that during our conversations while moving it. This is the comment from @plopesc:

Creating a new twig extension would be much simpler, but in the long term, I believe that will be better to have a single one.
I'll work on the BC approach for now.

Do you think that reverting the changes to TwigExtension and moving the SDC Twig extension to Drupal\Core\Template\ComponentsTwigExtension is a better approach? I felt good about both options back in the day, so I'll be happy to implement it.

The relevant conversations can be found in 📌 Move SDC Twig\TwigExtension to Core namespace Active , and its corresponding MR.

The library prefix - do we need to consider BC?

Yes. This is how we tried to handle it. This section of the code generates the libraries with the old names for backwards compatibility reasons.

I see that there is a small error with the wording of the deprecation message that I will fix right away.

The relevant conversations can be found in 📌 Move SDC sdc.module functions to Core namespace Fixed and in its corresponding MR.