Problem/Motivation

According the the documentation we should be able to do the following:

    tertiary:
        type: string
        title: Tertiary

        # Limit the available options by using enums.
        enum:
            - success
            - warning
            - danger

        # Provide a default value
        default: success

However this doesn't seem to be implemented. In the tests this value is missing from the my-button component and is being passed in explicitly through the template instead.

Steps to reproduce

Failing test attached, see MR !5032 with the failing test report.

Proposed resolution

Implement the missing default functionality for SDC, see MR !5353, including both the test change, and the actual fix.

Remaining tasks

Maintainer review.

User interface changes

No.

API changes

No.
Documentation always suggested `default` should work for properties, so it is instead allowing that behavior.

Data model changes

No.

Release notes snippet

Maybe, hinting that default can be used now, as originally intended is a good idea, but may not be needed.
Also, let us change the related documentation to remove the "It isn't used in Twig template." around default, which would be implemented after this change gets in.

CommentFileSizeAuthor
#13 Screenshot_20240416_122134.png710.55 KBe0ipso

Issue fork drupal-3394815

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

justafish created an issue. See original summary.

justafish’s picture

Issue summary: View changes

wotnak made their first commit to this issue’s fork.

wotnak’s picture

Status: Active » Needs review

Added props default value functionality implementation in https://git.drupalcode.org/project/drupal/-/merge_requests/5353. It passes test modified by justafish.
I implemented it in `sdc_additional_context` twig function that is executed in ComponentNodeVisitor and already used to add things to component context. The implementation loops through component props and if a given prop has a default value set and doesn't already have a value in the current context, the default value is added to the context (which makes it available in the component template).

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs issue summary update

Can the issue summary be updated please, left some TBD if they need to be filled in, can put NA if it doesn't apply to this issue.

FrankieD3’s picture

Any updates on this functionality? If this has still yet to be implemented, the documentation should be altered to reflect the current state of the module so not to mislead developers.

marvil07’s picture

Title: "default" functionality isn't implemented » Component "default" functionality isn't implemented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

@justafish, thanks for opening this issue, and providing a clear test change to reproduce it 👍

@wotnak, thanks for implementing a fix! 👍

Apart from the test fix, I also verified with another use-case on a custom theme that the default value is being used.

@smustgrave, I have updated the issue summary with a few more details.

Marking as RTBC.

nod_ made their first commit to this issue’s fork.

e0ipso’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
710.55 KB

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

Screenshot of a Slack conversation

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. As brought up in #8.

[...] the documentation should be altered to reflect the current state of the module so not to mislead developers.

I wonder if adding a section in the official FAQs is the way to fix this.

alexpott’s picture

We need to update https://www.drupal.org/docs/develop/theming-drupal/using-single-director... for sure as that's what as caused some of the confusion.

agentrickard’s picture

I tend to agree with the assessment that this is a documentation/schema feature and not a means to inject data into Twig, which would make this a documentation issue.

I wonder if this part: generating synthetic example renderings of a component is something that might be implemented by modules like Storybook. If so, we could open a similar issue there.

pdureau’s picture

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.

🙏

The current recommendation is to use Twig to do so.

As a side note, using default() filter is a better practice than the collapsed (?:) or null (??) ternary operator:

FrankieD3’s picture

I feel as though the updated text "Provide a default value. It isn't used in Twig template." does not include enough context. If I were looking at this with fresh eyes, I would be thinking to myself, "then where is it being used?"

An example of a Twig template containing the use of the default() filter would be useful somewhere within the documentation. Something to the effect of {% set text = text|default('Placeholder'|t) %}.