Can Picafort
Account created on 11 July 2009, about 15 years ago
#

Merge Requests

More

Recent comments

e0ipso Can Picafort

Thanks @apaderno. Is there any way to update the Created by field to use my user instead?

e0ipso Can Picafort

Thanks @apaderno!

Is there any way to remove this reference?

e0ipso Can Picafort

Hello.

It's been more than 14 days now. Did the maintainers respond to the queries?

e0ipso Can Picafort

One small suggestion, do not ever edit an accepted record. Only make cosmetic changes to it. If you need to change it, deprecate it and write a new one. Write why you are deprecating it on the new one. Links back and forth should be present in both ADRs.

WRT #12 we have seen much better results when the ADRs are in the repo. In our experience, putting them as close as possible to the code helps developers maintain them.

e0ipso Can Picafort

I think people can probably figure out that "Single Directory Components" in a slide deck refers to a thing that is officially called "Single-Directory Components".

Agreed. This is exactly what I meant when I said I think this is a minor issue, so I side with the inconsistency., but more eloquently put.

e0ipso Can Picafort

If you are curious about my confusion in #8, I was trying to write a test for 4, before I wrote one for 2:

4. Have embed within extends and forward blocks from the parent to the children.

However, that is not valid Twig. And that is what a I was realizing in #8.

The commit above adds test coverage for extends, and we should be good now.

e0ipso Can Picafort

Please ignore my comment in #8 🀦. I was conflating two different issues here.

To recap this issue proves that we can:

  1. Have embed within embeds.
  2. Have embed within extends.
  3. Have embed within embeds and forward blocks from the parent to the children.
e0ipso Can Picafort

This is good for me. I didn't know about this grammatical rule, and it didn't come up earlier in any of the reviews.

My only question is about documentation. The status quo is that code and documentation are consistent bur wrong. Changing this in code is nice and easy, but now we have inconsistent documentation (official Drupal.org docs, video keynotes, DrupalCon presentations, DrupalCamp presentations, contrib project pages, slack messages, etc.)

I am OK with the change, and I will try to use the new for moving forward, if we agree on inconsistent docs. For the record, I think this is a minor issue, so I side with the inconsistency.

e0ipso Can Picafort

To make it clearer: I would not consider the intention to commit code in the next 30 days a reason sufficient not to transfer the project ownership, considering that the project node does not contain any information about the project.

Could this be a reason to speed up the process?

e0ipso Can Picafort

Moved to the project ownership issue queue after 2 weeks without a response (even if this qualifies as an "empty" project).

e0ipso Can Picafort

@alexpott while working on an example with extends I realized that the trick to forward block contents will not work on extended templates. This is not related to SDC, but a Twig limitation.

{# extends-example.twig #}
{% extends 'sdc_test:just-a-slot' %}
{% set slot_value %}
  {% block nested_slot %}{% endblock %}
{% endset %}

{% block the_slot %}Foo {{ slot_value }}{% endblock %}

In the example above there is no scope for extend-example.twig to grab the contents of the block nested_slot.

Since we are extending another template it does not make sense to introduce a new slot/block, you just use the blocks defined by the parent instead. Therefore I think now, that I was wrong in #2 to suggest that we need to support extends.

Back to RTBC based on the above.

e0ipso Can Picafort

Thanks! I added this to the project page.

e0ipso Can Picafort

For the record, I reached out to Mustafa Hajmohammed (mustafa.hajmohammed) three more times after the comment above without a reply, so far.

I contacted the user:

  1. 28/05/2024
  2. 29/05/2024
  3. 31/05/2024
e0ipso Can Picafort

I have been doing this for a while in Drupal using https://github.com/e0ipso/schema-forms-php

However, I think that the current approach is geared towards using field widgets instead. In other words, instead of mapping the schema to the form elements, it tries to map the schema to the typed data in field types. Once you have the field type, you can load the widget for it.

e0ipso Can Picafort

Test only patch is failing with the expected error: https://git.drupalcode.org/issue/drupal-3446933/-/jobs/1730262#L45

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("We found an unexpected slot that is not declared: [the_slot]. Declare them in "my-banner.component.yml".").

The pipeline with the fix is green.

This is ready to review.

e0ipso Can Picafort

At this point, it feels that this issue is only about a name convention. Variant is a regular prop: we can do that today. The template is managed manually: we can do that today.

The only remaining value is to ensure that people uses variants with the same name, and thus it becomes introspectable metadata that we can use down the line (add a selector in XB?).

Should we update the IS?

e0ipso Can Picafort

In the beginning, all of these actions are achieved through a low-code experience using a code editor within the browser.

I have to admit I can't get over my initial reaction about a "code editor within the browser", it gives me pause. What is the rationale behind this requirement? I would prefer, if possible, to not do this and jump straight to the no-code solution, without this stepping stone.

The design system may also have no-code configuration options (e.g. color schemes, fonts, spacing, etc) to enable non-technical builders to create experiences tailored to their brand and content strategy.

Yes!

Exposing a component to the Experience Builder should be as easy as it is with competing products.

Any examples of what it is like with competing products?

Thanks for this writeup @lauriii. My favorite part is that technical-builders and low-code builders are on the same track, even if using different tools. I think that having significantly different theme building experience would have eventually sidelined one or the other. I know we are focusing on the ambitious site builder persona, but at the end of the day there are lots and lots of technical builders building Drupal sites that we need to keep prioritizing. I see this vision supporting that.

e0ipso Can Picafort

From experience, I believe most of the use of variants will be in class names, as BEM modifier or equivalents in other naming methodologies:

AFAICT you can accomplish that with a regular prop today, no need for this ticket to land.

this feature is optional (I don't have to create different template every time I create a new variant)

Couldn't you load templates manually from the main template based on the variable? E.g. {% include "banner--" ~ variant ~ ".html.twig" %".

Just pointing out that this two statements may appear to be in conflict, but I believe they are not. We need to ensure the template file exists before doing the template swap.

the variant prop will be injected into the template

I think this may belong to the componentMetadata object that gets injected. Thoughts?

I am still unconvinced about making the user request the variant using a seemingly regular prop. It feels like bad UX. I only see two inputs for embedding a component: the template name (my_module:my-component) and the template context (with {...}), so we don't have many options.

If we are to introduce props with render logic, perhaps we could introduce an unlikely reserved prop for those. Then users would write:

{% embed 'my_theme:cool-component' with {
  unlikelyReservedNameUpForBikeshedding: { variant: 'my-variant-name' },
  prop1: 'foo',
  prop2: 'bar',
} %}

In the future we could add more stuff in there (aside from variants), should we need to do so.

e0ipso Can Picafort

Mustafa Hajmohammed (mustafa.hajmohammed) just replied to the email sent on 24/05/2024 agreeing to the transfer. He will take action shortly.

e0ipso Can Picafort

Thanks for the vote of confidence! I can confirm that I have read the sections linked by @xjm in #12, and I agree to follow the principles in them.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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.

e0ipso Can Picafort

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...

e0ipso Can Picafort

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

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

There is also the fact that we'll download components, and we will need to tweak them later on. For this to be possible a component will need to have their schema in the component definition.
e0ipso Can Picafort

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.

e0ipso Can Picafort

🀣 this made me laugh @mherchel!

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

e0ipso Can Picafort

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

e0ipso Can Picafort

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?

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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)

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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).

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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 Can Picafort

Adding a reference to the sponsor's logo.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

It's stable now!

e0ipso Can Picafort

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.
e0ipso Can Picafort

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

e0ipso Can Picafort

Update docs after 10.3 changes.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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.

e0ipso Can Picafort

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 .

e0ipso Can Picafort

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

e0ipso Can Picafort

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

[dΓ©jΓ  vu]

e0ipso Can Picafort

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

e0ipso Can Picafort

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

e0ipso Can Picafort

I agree with the main sentiment that the shortcut module has the potential to be very useful. I agree with these proposals. πŸ‘πŸ‘πŸ‘

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

e0ipso Can Picafort

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.

e0ipso Can Picafort

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.

e0ipso Can Picafort

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

Production build 0.69.0 2024