Enum vales do not have translatable labels

Created on 11 December 2024, 6 months ago

Problem/Motivation

'enums values do not have (translatable) labels'

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Feature request
Status

Active

Version

11.0 🔥

Component

single-directory components

Created by

🇦🇺Australia griffynh Sydney

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @griffynh
  • 🇦🇺Australia griffynh Sydney
  • 🇦🇺Australia griffynh Sydney
  • 🇫🇷France pdureau Paris

    For your information, in UI Patterns 2 , we use "meta:enum" which is not an official standard but supported by some popular projects:

    props:
      type: object
      properties:
        position:
          type: string
          enum:
            - top
            - bottom
          "meta:enum":
            top: Top
            bottom: Bottom
    

    If an item is in enum but not in meta:enum, its label will be the item string
    If an item is in meta:enum but not in enum, it is ignored.

    It would be great to stay compatible.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    @pdureau You're an SDC maintainer now, so … let's just get #4 implemented? 😄 Happy to provide reviews!

  • 🇫🇷France pdureau Paris

    So, we have agreed on using meta:enum and leveraging this information and the related translations (from locale module API) is up to the display building tools like UI Patterns 2 and Experience Builder.

    Also, adding meta:enum to the documentation will be done in this issue 🌱 Clarify SDC documentation by toning down Twig blocks promotion Active .

    So, what can we do in Core?

    • Add a mention of meta:enum in ComponentMetadata? Ans some light logic?
    • Add meta:enum to some of our test components
    • Add a specific test about meta:enum? which one?
  • 🇺🇸United States mradcliffe USA

    I performed Novice Triage on this issue. I added the Novice issue tag because we can update the issue summary and potentially start. We need to come up with a good test for the change.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    #4 referenced https://github.com/adobe/jsonschema2md, so I went to look for an example there, and found one that's sufficiently silly to be fun:

        "string_pattern": {
          "type": "string",
          "description": "A string following a regular expression",
          "pattern": "^ba.$",
          "examples": ["bar", "baz", "bat"],
          "meta:enum": {
            "baa": "the sounds of sheeps",
            "bad": "German bathroom",
            "bag": "holding device",
            "bah": "humbug!",
            "bam": "a loud sound",
            "ban": "don't do this",
            "bap": "a British soft bread roll",
            "bas": "from ancient Egyptian religion, an aspect of the soul",
            "bat": "…out of hell",
            "bay": ", sitting by the dock of the"
          },
    

    https://github.com/adobe/jsonschema2md/blob/f3b5773eb610130891503c1cf71b...

    So let's use that one (or a variation thereof).

    By the way, Add an icon management API Active also used meta:enum:

     * If an `enum` is set, then the select is used:
     * - enum => #type = select and #options
     * The key `meta:enum` is used to support description for each enum.
     *
     * @internal
     *   This API is experimental.
     */
    class IconExtractorSettingsForm {
    
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Updated issue summary per @pdureau's #8.

    Expanded it to a full implementation plan. Which is why it's clear this is definitely not .

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • First commit to issue fork.
  • Pipeline finished with Failed
    about 2 months ago
    Total: 99s
    #470051
  • Pipeline finished with Failed
    about 2 months ago
    Total: 162s
    #470126
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • Pipeline finished with Failed
    about 2 months ago
    Total: 100s
    #473005
  • Pipeline finished with Failed
    about 2 months ago
    Total: 114s
    #473018
  • Pipeline finished with Failed
    about 2 months ago
    Total: 100s
    #473043
  • Pipeline finished with Failed
    about 2 months ago
    Total: 569s
    #473046
  • Pipeline finished with Failed
    about 2 months ago
    Total: 633s
    #473063
  • Pipeline finished with Failed
    about 2 months ago
    Total: 115s
    #473093
  • Pipeline finished with Success
    about 2 months ago
    Total: 1216s
    #473104
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    This is looking GREAT! 🤩

  • Pipeline finished with Canceled
    about 2 months ago
    Total: 541s
    #473184
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 224s
    #473195
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 137s
    #473200
  • Pipeline finished with Success
    about 2 months ago
    Total: 608s
    #473201
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    @pdureau: as an SDC maintainer, what do you think about https://git.drupalcode.org/project/drupal/-/merge_requests/11791/diffs#n...?

  • Pipeline finished with Success
    about 2 months ago
    Total: 339s
    #473757
  • Pipeline finished with Failed
    about 2 months ago
    Total: 194s
    #473935
  • Pipeline finished with Failed
    about 2 months ago
    Total: 118s
    #473943
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 149s
    #473948
  • Pipeline finished with Failed
    about 2 months ago
    Total: 795s
    #473954
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Added an end-to-end test (as a kernel test). This is ready from my POV.
    The test failure is happening in HEAD too.

  • Pipeline finished with Success
    about 2 months ago
    Total: 1730s
    #474036
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    FYI: Ran into @pdureau at DDD yesterday — he +1'd the implementation plan I added to the issue summary in #11 😊

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    I think this is in the final stretch now! This needs a change record, plus 2 clarifications, plus 1 code clarity nit by Lee that I +1'd.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Fixed everything from Wim review, added change record draft.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 1061s
    #475304
  • Pipeline finished with Success
    about 2 months ago
    Total: 363s
    #475461
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 93s
    #479633
  • Pipeline finished with Success
    about 1 month ago
    Total: 2258s
    #479634
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 410s
    #479694
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 82s
    #479696
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 72s
    #479697
  • Pipeline finished with Failed
    about 1 month ago
    Total: 163s
    #479698
  • Pipeline finished with Success
    about 1 month ago
    Total: 1731s
    #479699
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    I think this is ready

  • 🇺🇸United States phenaproxima Massachusetts
  • 🇬🇧United Kingdom longwave UK

    Added some questions to the MR.

  • Status changed to Needs work about 1 month ago
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇬🇧United Kingdom longwave UK

    Discussed this with @penyaskito and @effulgentsia. We came up with two alternative options; we can still bikeshed over the exact names but JSON Schema has no opinion on how to handle extensions, so we need to make our own decision here.

    1. As proposed by @pdureau in #4 we use meta:enum for human readable enum values, and some other meta: prefix key for translation contexts.
    2. Or, as per this JSON Schema blog post where we added x-formatting-context to Experience Builder 📌 `StringSemanticsConstraint::MARKUP`: agree how SDC prop JSON schema can convey it should be markup, and Needs review we use something like x-enum-labels for human readable enum values and x-translation-contexts
    3. for translation contexts.

    @pdureau (or anyone else!) do you have any strong preferences or guidance on this?

  • 🇫🇷France pdureau Paris

    I would prefer a policy where we avoid as much custom JSON schema annotation as possible:

    • They are creating our own "island" in the JSON schema world
    • They are tempting to be used as convenient workarounds on the SDC side instead of fixing the root cause in the application side.
      x-formatting-context is an example of such workaround, because we are asking the component author to add an annotation related to the UX of a specific display building tool he doesn't have to care about, instead of being focused only on its component's own UI model and logic.

    That's why I like meta:enum. Yes, it is not part of the JSON schema specs, however:

    • it is not something we have invented, it is already used in opens source tools
    • it is still describing the component and not some unrelated applicative stuff
    • it looks like something special, we are not opening a Pandora box by normalizing x- annotations
    • it looks like it can be included in a future version of JSON schema
  • 🇺🇸United States effulgentsia

    That's why I like meta:enum. Yes, it is not part of the JSON schema specs, however...it is not something we have invented, it is already used in opens source tools

    But is that future-compatible considering https://json-schema.org/blog/posts/the-last-breaking-change? When a stable json schema version is released and tools adopt it, are we expecting https://github.com/adobe/jsonschema2md to provide the vocabulary/schema for it, and then we'll change all of our SDCs to reference it?

    If we do stick with meta:enum given its prior art, then what about translation context? If that's one that we are inventing here and not copying from other OSS tools, then I think that one needs an x- prefix given JSON schema's move away from unknown keywords not prefixed with that. Perhaps x-translation-context?

  • 🇺🇸United States effulgentsia

    x-formatting-context is an example of such workaround, because we are asking the component author to add an annotation related to the UX of a specific display building tool he doesn't have to care about, instead of being focused only on its component's own UI model and logic

    My reply to this is tangent to this issue, but I do want to note that I disagree with this characterization. If an SDC defines an HTML-containing prop, and then has Twig code that renders that prop inside a <p> tag or has CSS that assumes that what's in that prop is only inline formatted content, then it's the component's own UI logic that dictates x-formatting-context: inline.

  • 🇺🇸United States effulgentsia

    and then we'll change all of our SDCs to reference it?

    Maybe that won't be necessary since SDCs reference the schema in Drupal core, so we'll be able to update just that central one when the time comes?

  • 🇫🇷France pdureau Paris

    an HTML-containing prop

    You mean a slot? 😉

  • Pipeline finished with Failed
    30 days ago
    Total: 503s
    #491367
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Thanks everyone for the reviews and the conversations.

    Given all the arguments, I'm gonna continue with the implementation with these in mind:

    • We want to use "meta:enum", even if not very future-proof, for consistency with other frameworks frontend devs might be familiar with.
    • "meta:enum" shouldn't be required. We default to the enum keys if not provided.
    • We will add x-translation-context at the prop level. We will take into account this might get promoted to the component level, so we want to "cascade". If no translation context found, we use an empty string.

    So an example of an enum prop would be:

          enum:
            - info
            - success
          meta:enum:
            info: Information
            success: Success
          x-translation-context: "Status icon"
    
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Update IS with latest discussions agreements.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇬🇧United Kingdom longwave UK

    +1 for the decision in #38, given we have seen prior art in using meta:enum but that also the x- prefix is seemingly preferred by the JSON Schema team for extensions.

    I think cascading the context makes sense too. It seems likely that you would want the same context to be applied across all translatable strings in the prop (and certainly all cases in the enum), but you might also want it to cover the whole component without having to repeat yourself.

    @pdureau are you OK with this?

  • Pipeline finished with Failed
    30 days ago
    Total: 121s
    #491572
  • Pipeline finished with Canceled
    30 days ago
    Total: 139s
    #491601
  • Pipeline finished with Failed
    30 days ago
    Total: 253s
    #491603
  • Pipeline finished with Canceled
    29 days ago
    Total: 638s
    #491620
  • Pipeline finished with Canceled
    29 days ago
    Total: 233s
    #491625
  • Pipeline finished with Canceled
    29 days ago
    Total: 190s
    #491629
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Needs review again.

  • Pipeline finished with Failed
    29 days ago
    #491633
  • Pipeline finished with Success
    29 days ago
    Total: 2542s
    #491653
  • The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Don't know why the bot complained, gitlab didn't, and rebase was automatic:

    $ git rebase 11.x
    Successfully rebased and updated refs/heads/3493070-sdc-metaenum.
    
  • Pipeline finished with Success
    25 days ago
    Total: 693s
    #495084
  • Pipeline finished with Success
    23 days ago
    Total: 786s
    #496431
  • Pipeline finished with Success
    14 days ago
    Total: 398s
    #504447
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Rebased again, guess too late for 11.2.0 now tho.

  • Pipeline finished with Success
    9 days ago
    Total: 637s
    #507953
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Discussed with release manager if this could still make it into 11.2 because of the strategic importance.

    There are some new exceptions thrown but the change is not disruptive - it only occurs if you opt in to the new feature but declare your meta values incorrectly.

            if (isset($prop_schema['enum'], $prop_schema['meta:enum'])) {
              $enum_keys_diff = array_diff($prop_schema['enum'], array_keys($prop_schema['meta:enum']));
              if (!empty($enum_keys_diff)) {
                throw new InvalidComponentException(sprintf('The values for the %s prop enum in component %s must be defined in meta:enum.', $name, $this->id));
              }
            }
    
    
  • 🇬🇧United Kingdom catch

    xjm credited catch .

  • 🇺🇸United States xjm

    Specifically, while a change like this would normally need to be committed before 11.2.0-beta1, we're willing to allow it up until 11.2.0-rc1 given the impact on the XB release cycle and the fact that this was made into a minimally disruptive API addition per #46.

    However, if this isn't fixed before RC1, it will still have to wait for 11.3 unfortunately, because it is still a minor-only feature and API addition. RC1 is scheduled for this week. :) @larowlan can hopefully keep the RMs updated on where we're at with this issue up until the RC is tagged. Thanks all!

  • 🇫🇮Finland lauriii Finland

    xjm credited lauriii .

  • 🇺🇸United States xjm

    Meant to credit Lauri also for challenging whether it was still disruptive.

  • 🇳🇿New Zealand danielveza Brisbane, AU

    Left a review, mainly around the patterns for the meta:enum property and some small test questions

  • Pipeline finished with Canceled
    1 day ago
    #514670
  • Pipeline finished with Canceled
    1 day ago
    Total: 71s
    #514674
  • Pipeline finished with Failed
    1 day ago
    Total: 167s
    #514675
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Issue credits

  • Pipeline finished with Failed
    1 day ago
    Total: 513s
    #514679
  • Pipeline finished with Canceled
    1 day ago
    Total: 74s
    #514681
  • Pipeline finished with Success
    1 day ago
    #514682
  • 🇳🇿New Zealand danielveza Brisbane, AU

    This issue has gone through a number of comprehensive reviews, the tests are green and all comments from my most recent review have been fixed or commented on.

    I think this is ready to be in RTBC.

    • larowlan committed 65f2db7e on 11.2.x
      Issue #3493070 by penyaskito, griffynh, wim leers, longwave, pdureau,...
    • larowlan committed 9d55d1e6 on 11.x
      Issue #3493070 by penyaskito, griffynh, wim leers, longwave, pdureau,...
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Committed to 11.x and backported to 11.2.x per #48

    Published the change record. Thanks everyone 💙

  • 🇺🇸United States xjm

    Improving the release note to link the CR and explain the importance of the change in the broader context.

Production build 0.71.5 2024