Overview

Enum vales do not have (translatable) labels.

enums panel in XB

We need human-readable equivalents, and those equivalents must be translatable (using Drupal's interface translation mechanism).

⚠️ This should not be XB-specific.

⚠️ Out-of-scope here: translatability. Nothing in XB is translatable yet today. This will add the necessary infrastructure to make it feasible later 👍

Proposed resolution

  1. Fix this in core first: implement #3493070-10: SDC `enum` props should have translatable labels: use `meta:enum`.
  2. Then, port that core MR to XB, with the following changes:
    1. XB should perform the equivalent of these in \Drupal\experience_builder\ComponentMetadataRequirementsChecker:
      1. INFRA: Update \Drupal\Core\Theme\Component\ComponentMetadata::parseSchemaInfo() to trigger a deprecation error when an enum is encountered without a corresponding meta:enum
      2. INFRA: Update \Drupal\Core\Theme\Component\ComponentMetadata::parseSchemaInfo() to trigger a ComponentDoesNotMeetRequirementsException when a meta:enum is encountered whose keys do not match (i.e. are a subset or superset) the values listed in the corresponding enum.

      … but rather than deprecating or a logic exception, it will simply become a hard requirement for XB.

    2. XB must support this not only for SDCs, but also for its "in-browser code components" (aka JavaScriptComponent config entities), which use the same metadata structure as SDCs.

      For that, see the attached config schema patch.

      If there is no meta:enum present, the value will fallback to the enum.

    3. Modify \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testJavaScriptComponent() to also create a enum prop. This should first fail validation (422 response), then upon adding the matching meta:enum, it should pass validation.

      This is XB's equivalent for

      1. TEST: kernel test asserting that a meta:enum not matching the enum triggers a ComponentDoesNotMeetRequirementsException
    4. XB must change the logic in \Drupal\experience_builder\JsonSchemaInterpreter\SdcPropJsonSchemaType::toDataTypeShapeRequirements() to do the equivalent of
      1. INFRA: add a getEnumOptions(string $prop): array<string, TranslatableMarkup> method to \Drupal\Core\Theme\Component\ComponentMetadata inspired by ui_patterns' \Drupal\ui_patterns\EnumTrait::getEnumOptions(). (The thing that ui_patterns does not yet do is pass it through Drupal's t().)

      That means changing e.g.

            SdcPropJsonSchemaType::STRING => match (TRUE) {
              array_key_exists('enum', $schema) => new DataTypeShapeRequirement('Choice', [
                'choices' => $schema['enum'],
              ], NULL),
      

      to actually use the meta:enum values.

      (Note that here, they must NOT be translated — translation must happen at runtime — this corresponds to a FieldConfig config entity aka configuration.)

    5. Now update ui/tests/e2e/prop-types.cy.js to prove that the labels for the enum indeed appear in the UI, by updating the it('Enum (select) - string', () => { test case.

      This is XB's equivalent for

      1. TEST: kernel test asserting that 2 identical type: string, enum: […] props can have different translations for the same enum values. For example: a '' enum value results in Same window for the target prop and in None for a rel prop.
        → verifies it works end-to-end, and supports translation contexts

User interface changes

  • Any SDC that has enum without the necessary meta:enum will no longer be available to XB users, and will now appear in the list of disabled XB components at /admin/appearance/component/status, with an explanation of why.
  • Any SDC (or code component) that does have both enum and a matching meta:enum will now see those labels in the component inputs form on the right-hand side.
  • Any SDC (or code component) that does have enum but not meta:enum will have the enum values. (eg. my-banner or my-cta in core)

Steps to test

Is useful to enable xb_test_sdc and _sb_test_code_components modules. There are created components with and without meta:enums and with mismatched meta:enums.

  • Include a SDC with enum and meta:enum configured and confirm that the values that appear are the labels configured in meta:enum
  • Include a SDC with enum and without meta:enum configured and confirm that the values that appear are the values configured in enum
  • Enable the module xb_test_sdc, navigate to /admin/appearance/component/status and confirm the reason why sdc.xb_test_sdc.component-mismatch-meta-enum wasn't enabled is because the enum and meta:enum values mismatch
  • In both components, modify the values and confirm that the changed values preview correctly but is not changed in the page without saving.
  • Confirm that in both cases, after saving the page (review x changes, publish all changeS), the values display correctly.
  • Create a new code component (can be in a module inside config/install and can be the one I've attached) Enable the xb_test_code_components_with_enums code component and confirm that the field with meta:enum has Capitalized values, and the one with only enum has the plain values. Play around by adding or removing enums and meta:enums in the config (in that case the component must be first exported to config/sync and be modified there.
  • Confirm the same tests in this code component than the ones done in the Single Directory Component

Follow-up needed:

  • UI for defining key and values for enums in js components
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:

Comments

wim leers created an issue. See original summary.

wim leers’s picture

Title: Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum` » [PP-1] Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum`
Issue summary: View changes
Status: Active » Postponed
StatusFileSize
new2.03 KB

Config schema changes necessary for XB's JavaScriptComponent config entities attached to help get this going 👍

wim leers’s picture

Issue summary: View changes
Issue tags: +Needs followup

wim leers credited pdureau.

wim leers’s picture

Issue summary: View changes

Fixing markup.

And crediting @pdureau, for our collaboration at DrupalCon Atlanta to ensure XB and UI Patterns 2 are aligned 😊

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

wim leers’s picture

Title: [PP-1] Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum` » Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum`
Status: Postponed » Active

Exciting to see #3493070: SDC `enum` props should have translatable labels: use `meta:enum` be ready for review! Now let's get this going 😁 (Because XB can't wait for core to ship this in a release.)

isholgueras’s picture

Assigned: Unassigned » isholgueras
isholgueras’s picture

Issue summary: View changes
isholgueras’s picture

Assigned: isholgueras » penyaskito
penyaskito’s picture

Assigned: penyaskito » isholgueras
Status: Active » Needs review

This works and has good test coverage.

  • We need a follow-up for when the core issue lands.
  • We might want a follow-up to change the Code Components UI for having 2 fields and store option lists as "value => Human-friendly label" instead of having only the value as of now.
penyaskito’s picture

Re-reading IS I think Wim expected to implement the Code Components here too? 🤔🤔

lauriii’s picture

+1 for the follow-ups mentioned in #12.

penyaskito’s picture

It took me a while to figure out why we need any change at all for JS Component.
So documenting it step by step.

Right now, the site builder will generate their prop, and select "List: string".
They see a single textfield, named "Value", with placeholder "Enter a text value", where they input their values one by one, and select the default one.

That generates:

js_component.enumcodecomponent.yml
[...]
props:
  color:
    title: Color
    type: string
    examples:
      - green
    enum:
      - blue
      - green
      - red
      - Orange
      - 'Another weird color'

One we enable the component, that transforms to:

experience_builder.component.js.enumcodecomponent.yml
[...]
        allowed_values:
          -
            value: blue
            label: blue
          -
            value: green
            label: green
          -
            value: red
            label: red
          -
            value: Orange
            label: Orange
          -
            value: 'Another weird color'
            label: 'Another weird color'

So if they input a friendly label, that's what they get. For translation, we need to translate that config entity.
There's no way we can do this right without having some value - label pair on the client, or document how their values will be generated (potentially like prop names already are). In that case we might indicate in the UI that we expect labels, not values, and ideally print the value so they can copypaste that into the code editor.

If we do that in the client, they will post the pairs (or the value can be easily calculated from the label, as prop names do). For simplicity, we use the same format than SDC (see props in js_component above).

And when we have that, it's the client who would send the enum and the meta:enum (optionally we might want a x-translation-context, or generate one server side, but that's definitely for another follow-up), and we will have the same info than a "meta:enum" complete component (or an easy way to generate it server-side), which will generate the right config entity allowed_values pairs for e.g. translation.

penyaskito’s picture

Assigned: isholgueras » wim leers

I think this is ready if we split the code components part, which I moved to #3523680: JsComponents `enum` props should have human-readable labels: use `meta:enum`

penyaskito’s picture

Assigned: wim leers » penyaskito
Status: Needs review » Needs work

We'll work on JsComponents here.

penyaskito’s picture

Assigned: penyaskito » wim leers
Status: Needs work » Needs review
isholgueras’s picture

Issue summary: View changes
isholgueras’s picture

Issue summary: View changes
StatusFileSize
new1.09 KB
isholgueras’s picture

Issue summary: View changes
isholgueras’s picture

Assigned: wim leers » isholgueras
Status: Needs review » Needs work
isholgueras’s picture

Assigned: isholgueras » wim leers
Status: Needs work » Needs review

I wanted to add a new code component with 2 props, one with enums and another one with enums and meta:enums to have a yml file with this as an example.

We had tested it in XbConfigEntityHttpApiTest but I think having a code component in a yml is useful for testing and for new adopters to see how is done.

I've also fixed issues with HEAD.

Steps to test, in Summary, is also updated.

isholgueras’s picture

Assigned: wim leers » isholgueras
Status: Needs review » Needs work

It needs to fix a complex conflict.

isholgueras’s picture

Assigned: isholgueras » wim leers
Status: Needs work » Needs review
wim leers’s picture

Assigned: wim leers » isholgueras
Status: Needs review » Needs work

Partial review, but I'm pretty sure I spotted some significant simplification potential? 😇

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

penyaskito’s picture

wim leers’s picture

#28: 🎉🥳

Next up: make XB leap ahead of core by landing this MR … ⚠️ but because XB will not be able to require 11.2 until AFTER beta1, we need to be careful about how we approach it.

wim leers’s picture

Assigned: isholgueras » Unassigned

This needs to make meta:enum mandatory for XB. 🙏 See the rationale.

wim leers’s picture

Title: Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum` » Leap ahead of #3493070 in Drupal 11.2: SDC `enum` props should have human-readable labels: use `meta:enum`
penyaskito’s picture

Assigned: Unassigned » penyaskito

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

wim leers’s picture

Merged in upstream, let's get this to passing tests so we can land it 🙏

isholgueras’s picture

Assigned: penyaskito » isholgueras
isholgueras’s picture

Conflicts with 0.x fixed

wim leers’s picture

Assigned: isholgueras » Unassigned

@isholgueras is now (hopefully) AFK :)

penyaskito’s picture

Assigned: Unassigned » balintbrews

All tests passing 🎉

Something that doesn't have e2e tests is the code editor, but we need to fix it to at least send the same values in enum as meta:enum.
Bálint volunteered to write this.

@balintbrews For now I'd expect to send the same added values as labels. The only thing to take into account is replacing dots with underscores. Something like:

const getMetaEnumValue = (x: string) => x.replace('.', '_');

const enumValues = [
      '3.14',
      'b',
      'c',
    ];
const metaEnums = Object.fromEntries(new Map(enumValues.map(value => [getMetaEnumValue(value), value])))
console.log(metaEnums);


//{
//  "3_14": "3.14",
//  "b": "b",
//  "c": "c"
//} 
lauriii’s picture

Any SDC that has enum without the necessary meta:enum will no longer be available to XB users, and will now appear in the list of disabled XB components at /admin/appearance/component/status, with an explanation of why.

I'm not entirely convinced that we should not allow SDCs without meta:enum to be used in XB because it is possible to achieve an acceptable UX without providing meta:enum, albeit not in all scenarios. For example, for margin-top, you could choose to provide options '4px', '6px', and '8px'. In this case the same value could apply both as the human readable and machine readable value.

We could still allow translating the enum options but keep the machine readable names consistent across translations or we could simply prevent translating these enums.

I don't think this needs to block this issue from being committed. It seems that we could loosen this requirement in future if this would require a non-trivial amount of work to change.

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

balintbrews’s picture

Assigned: balintbrews » penyaskito
Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Needs work

I need to self-review this yet, but feels close. Bálint changes look good to me, thanks!

pdureau’s picture

I'm not entirely convinced that we should not allow SDCs without meta:enum to be used in XB because it is possible to achieve an acceptable UX without providing meta:enum, albeit not in all scenarios. For example, for margin-top, you could choose to provide options '4px', '6px', and '8px'. In this case the same value could apply both as the human readable and machine readable value.

Hi Lauriii,

According to the #3528998: Follow-up: SDC `enum` props should have translatable labels: use `meta:enum`, all SDC prop with an enum has also a complete meta:enum. We made the meta:enum optional at the YAML declaration level, but the parseSchemaInfo() method is aligning the values:

         // Ensure all enum values are also in meta:enum.
          $enum = array_combine($prop_schema['enum'], $prop_schema['enum']);
          $prop_schema['meta:enum'] = array_replace($enum, $prop_schema['meta:enum'] ?? []);

Source: https://git.drupalcode.org/project/drupal/-/commit/166350f2acc649d60e4da...

penyaskito’s picture

Assigned: penyaskito » wim leers
Status: Needs work » Needs review

This is ready for further reviews.

wim leers’s picture

After raising a single (but very important because affecting Component versions and update paths for component instances) concern, then thinking I was wrong and reverting, then actually realizing I might've been right, it took me a few hours of digging deep into how the options module's field/widget infrastructure works, to find a better solution that overcomes all concerns and has the blessing of @penyaskito! (Just paired with him and demonstrated it.)

Let me illustrate it using XB's heading SDC's Component config entity's versions — start at the bottom of this example, because top = newest, bottom = oldest:

uuid: 6864c52b-71e4-4342-a825-0ce0f58b9533
langcode: en
status: true
dependencies:
  module:
    - options
active_version: 9616e3c4ab9b4fce
versioned_properties:
  active: 👈 👈👈 This represents the approach I refactored it to. No more labels, but also no more enum values.
              ⚠️ This is fine, because ComponentSourceBase::generateVersionHash() uses the normalized prop shapes to generate a deterministic hash, so changes in `enum` cause a new version, changes in `meta:enum` do not 👍
    settings:
      prop_field_definitions:
        element:
          field_type: list_string
          field_storage_settings:
            allowed_values_function: experience_builder_load_allowed_values_for_component_prop 👈 👈👈 No more labels!
          field_instance_settings: {  }
          field_widget: options_select
          default_value:
            -
              value: h1
          expression: ℹ︎list_string␟value
        style:
          field_type: list_string
          field_storage_settings:
            allowed_values_function: experience_builder_load_allowed_values_for_component_prop 👈 👈👈 No more labels!
          field_instance_settings: {  }
          field_widget: options_select
          default_value:
            -
              value: primary
          expression: ℹ︎list_string␟value
        text:
          field_type: string
          field_storage_settings: {  }
          field_instance_settings: {  }
          field_widget: string_textfield
          default_value:
            -
              value: 'A heading element'
          expression: ℹ︎string␟value
    fallback_metadata:
      slot_definitions: {  }
  ebbad79a7ac7d1fa: 👈 👈👈 This represents the state in the MR I started reviewing, after I broke it 😅
    settings:
      prop_field_definitions:
        element:
          field_type: string
          field_storage_settings: {  }
          field_instance_settings: {  }
          field_widget: string_textfield
          default_value:
            -
              value: h1
          expression: ℹ︎string␟value
        style:
          field_type: string
          field_storage_settings: {  }
          field_instance_settings: {  }
          field_widget: string_textfield
          default_value:
            -
              value: primary
          expression: ℹ︎string␟value
        text:
          field_type: string
          field_storage_settings: {  }
          field_instance_settings: {  }
          field_widget: string_textfield
          default_value:
            -
              value: 'A heading element'
          expression: ℹ︎string␟value
    fallback_metadata:
      slot_definitions: {  }
  94d7e3cb448df7c9: 👈 👈👈 This represents the state of the MR I started reviewing.
    settings:
      prop_field_definitions:
        element:
          field_type: list_string
          field_storage_settings:
            allowed_values: 👈 👈👈 Note the labels for the enum values are exactly what you'd expect!
              -
                value: div
                label: Container
              -
                value: h1
                label: Header 1
              -
                value: h2
                label: Header 2
              -
                value: h3
                label: Header 3
              -
                value: h4
                label: Header 4
              -
                value: h5
                label: Header 5
              -
                value: h6
                label: Header 6
          field_instance_settings: {  }
          field_widget: options_select
          default_value:
            -
              value: h1
          expression: ℹ︎list_string␟value
        style:
          field_type: list_string
          field_storage_settings:
            allowed_values:
              -
                value: primary
                label: Primary
              -
                value: secondary
                label: Secondary
          field_instance_settings: {  }
          field_widget: options_select
          default_value:
            -
              value: primary
          expression: ℹ︎list_string␟value
        text:
          field_type: string
          field_storage_settings: {  }
          field_instance_settings: {  }
          field_widget: string_textfield
          default_value:
            -
              value: 'A heading element'
          expression: ℹ︎string␟value
    fallback_metadata:
      slot_definitions: {  }
  1b4f8df7c94d7e3c: 👈 👈👈 This represents the state in HEAD.
    settings:
      prop_field_definitions:
        element:
          field_type: list_string
          field_storage_settings:
            allowed_values: 👈 👈👈 Note the labels for the enum values are … just the enum values. This is the terrible UX we want to fix.
              -
                value: div
                label: div
              -
                value: h1
                label: h1
              -
                value: h2
                label: h2
              -
                value: h3
                label: h3
              -
                value: h4
                label: h4
              -
                value: h5
                label: h5
              -
                value: h6
                label: h6
          field_instance_settings: {  }
          field_widget: options_select
          default_value:
            -
              value: h1
          expression: ℹ︎list_string␟value
        style:
          field_type: list_string
          field_storage_settings:
            allowed_values:
              -
                value: primary
                label: primary
              -
                value: secondary
                label: secondary
          field_instance_settings: {  }
          field_widget: options_select
          default_value:
            -
              value: primary
          expression: ℹ︎list_string␟value
        text:
          field_type: string
          field_storage_settings: {  }
          field_instance_settings: {  }
          field_widget: string_textfield
          default_value:
            -
              value: 'A heading element'
          expression: ℹ︎string␟value
    fallback_metadata:
      slot_definitions: {  }
label: Heading
id: sdc.experience_builder.heading
provider: experience_builder
source: sdc
source_local_id: 'experience_builder:heading'
category: Atom/Text

Next: make the MR pass again, by updating all component_versions all over the MR, because the different settings in the Component config entities that contain >=1 enum-shaped prop cause a different deterministic version hash 😅

wim leers’s picture

Assigned: wim leers » penyaskito
Status: Needs review » Reviewed & tested by the community

Green except e2e tests, hoping that @penyaskito can fix those. 😇

Epic work here! 👏

Still needs follow-up for updating the code component editor UI: https://git.drupalcode.org/project/experience_builder/-/merge_requests/8...

penyaskito’s picture

Status: Reviewed & tested by the community » Needs work

Didn't get to find why yet, but the enums aren't loading the default value/ set value correctly.

e.g.

  1. add a One Column SDC
  2. Set width to Narrow
  3. Select another component to navigate away
  4. Select the One Column component
  5. See width is not set to Narrow
penyaskito’s picture

What I've found is that this might be that we need some changes in the client UI that I didn't spot yet.

A symptom:

1. Add a heading component (has 2 enums)
2. Add a druplicon component
3. Edit the heading style + element props (enums)
4. Click on the druplicon. The form fails to load.
5. Back to heading, the style + elements have the defaults instead of our changes.

wim leers’s picture

Thanks, that's helpful. I suggest you switch to something else for a bit, it sounds like this has been a frustrating ride :/

penyaskito’s picture

I tested this and with this MR the values aren't set for any prop, not only enum, even when the SDC has no enums.

wim leers’s picture

Paired with @penyaskito on this. We got closer.

  1. Original state to start editing a page/article without anything in auto-save, the /xb/api/v0/api/layout/node/1 response contains this after placing a new Heading SDC:
  2. Then, upon changing the default value for the text prop, the client incorrectly sends this

    Note how the original prop value remains present (left) and the value I actually entered is sent with the wrong key (right).

This is what's causing things to not work. This appears to be happening in buildPreparedModel(), which is what generates the value for form_xb_props.

penyaskito’s picture

The last js component failures are because #3528998: Follow-up: SDC `enum` props should have translatable labels: use `meta:enum` avoids any way to provide meta:enums, overriding them even when set.

This is a problem when using \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::buildEphemeralSdcPluginInstance for validating js components as sdc components.

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

penyaskito’s picture

penyaskito’s picture

Assigned: penyaskito » wim leers
Issue summary: View changes
Status: Needs work » Needs review

Back to green. Added the followup needed in IS.

wim leers’s picture

Title: Leap ahead of #3493070 in Drupal 11.2: SDC `enum` props should have human-readable labels: use `meta:enum` » SDC `enum` props should have human-readable labels: use `meta:enum`
Assigned: wim leers » penyaskito
Status: Needs review » Needs work
wim leers’s picture

Assigned: penyaskito » Unassigned
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup

Finally. 🥹

  1. Create @todos pointing to a yet-to-be-created follow-up issue that drop XB's logic in favor of core's ComponentMetadata::getEnumOptions(), as well as any other things we could drop from XB once XB requires a core version containing #3493070.

This was added in #3 but is obsolete, because core didn't end up adding any such infrastructure 😅

wim leers’s picture

Status: Reviewed & tested by the community » Fixed
wim leers’s picture

Issue tags: +beta target

I mean, obviously :D

larowlan’s picture

Glad to see this go in. Mammoth effort

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

wim leers’s picture

This mammoth effort was indeed awesome to see land, but it didn't distinguish between SDCs and code components — it treated both equally.

That's how this ended up slowing down #3515074: Shape matching MUST work with the resolved equivalents of $refs AND must be compatible with core's upcoming $ref resolving in SDCs, because there a type: string, format: uri, enum: … test-only SDC was introduced. And the inevitable periods in URLs then were incorrectly deemed invalid.

So #3515074: Shape matching MUST work with the resolved equivalents of $refs AND must be compatible with core's upcoming $ref resolving in SDCs ended up refining what this did: it applied the "dot restriction" only to code components.