Added fix for <umb-button-group /> label issues

[abjerner]

Prerequisites

• I have added steps to test this contribution in the description below

As requested by @nul800sebastiaan, this PR targets the release/13.0 branch. My changes should however fix the issue both Umbraco 12.3 and 13.0.

Description

For Umbraco 12.3, @madsrasmussen made some changes to the <umb-button-group /> directive which adds support for labels on sub buttons. Previously a label was only supported on the default button: cb4d8e3

In my Skybrud Redirects Import package I had specified both a label and a label key for the sub buttons (as this worked on the default button). But with the changes to the <umb-button-group /> directive, both the label and the localized label associated with the label key are now shown.

This PR drops the use of the <localize /> directive as this doesn't support a fallback value based on an Angular expression. Instead the <umb-button-group /> directive will instead now use a watcher and the localizationService to resolve the localized label from the label key (if specified).

As trying to fix this, I also discovered two other issues:

• The value passed to the label parameter of the default button was {{defaultButton.labelKey}}, but as there is a separate label-key parameter, the passed value should be {{defaultButton.label}} instead.

• The screen reader text for the caret button was <localize key="{{labelKey}}">{{label}}</localize>. As far as I can tell, neither labelKey and label exists in the root of the scope, but instead lives as properties on the defaultButton object, and as described above, the <localize /> directive doesn't support Angular expressions as fallback, the fallback text would essential end up being {{label}} (not evaluated).

Fallback values

It's also worth mentioning that normally if a label key is passed to localizationService, but the label key isn't found, the resolved value will be the label key wrapped in square brackets. This will then overwrite the label (the fallback value). I've therefore added some checks for if (value && value.indexOf("[") === 0) return; so that the resolved value is ignored if it starts with an open square bracket character. This applies to the caret button as well as the sub buttons, but not the default button as this is instead handled by the <umb-button /> directive.

Since we've already handled the localization inside the <umb-button-group /> directive, removing this line should fix this as the defaultButton.label will contain the localized value. I haven't changed this for my PR, but I'd be happy to do so it makes sense to include here as well 😉

Testing

For testing this, I've set up a small dashboard with a two button groups. The files for the dashboard are:

• /App_Plugins/MyDashboard/Default.html

  <div ng-controller="myDashboard.controller as vm">
  
    <div style="padding: 50px 150px;">
      <umb-button-group
        ng-repeat="bg in vm.buttonGroups"
        button-style="{{bg.buttonStyle}}"
        default-button="bg.defaultButton"
        sub-buttons="bg.subButtons"
        direction="down"
        float="right">
      </umb-button-group>
    </div>
  
    <pre>{{vm | json}}</pre>
  </div>

• /App_Plugins/MyDashboard/package.manifest

  {
      "dashboards": [
          {
              "alias": "myDashboard",
              "sections": [ "content" ],
              "view": "/App_Plugins/MyDashboard/Default.html"
          }
      ],
      "javascript": [
          "/App_Plugins/MyDashboard/Controller.js"
      ]
  }

• /App_Plugins/MyDashboard/Controller.js

  angular.module("umbraco").controller("myDashboard.controller", function ($scope) {
  
    const vm = this;
  
    vm.buttonGroups = [
      {
        alias: "add",
        buttonStyle: "success",
        defaultButton: {
          labelKey: "redirects_addNewRedirect",
          label: "Add",
          handler: function () {
  
  
          }
        },
        subButtons: []
      },
      {
        buttonStyle: "success",
        defaultButton: {
          labelKey: "redirects_nonExisting",
          label: "Fallback",
          handler: function () {
  
  
          }
        },
      }
    ];
  
    vm.buttonGroups[0].subButtons.push({
      label: "Import",
      labelKey: "redirects_import",
      handler: function () {
  
  
      }
    });
  
    vm.buttonGroups[0].subButtons.push({
      label: "Export",
      labelKey: "redirects_import",
      handler: function () {
  
  
      }
    });
  
    vm.buttonGroups[0].subButtons.push({
      label: "Fallback",
      labelKey: "redirects_nonExisting",
      handler: function () {
  
  
      }
    });
  
  });

• /App_Plugins/MyDashboard/Lang/en-US.xml

  <?xml version="1.0" encoding="utf-8" standalone="yes"?>
  
  <language alias="en" intName="English (US)" localName="English (US)" lcid="" culture="en-US">
    <area alias="redirects">
      <key alias="addNewRedirect">Add new redirect</key>
      <key alias="import">Import</key>
    </area>
  </language>

The test should show the following:

• First sub button has an initial label of Import, which is overwritten by the resolved value from the label key (also Import).

• Second sub button has an initial label of Export, but to test the changes, I've specified the label key as redirects_import, the the resolved label is now Import (so that we can see that the label key does in fact overwrite the label).

• Third sub button has an initial label of Fallback. As no translation exists for redirects_nonExisting, the initial label is still shown (opposed to [redirects_nonExisting]).

• The second of the two primary buttons also has redirects_nonExisting as it's label key, but as the localization is handled by the <umb-button /> directive, and not <umb-button-group />, the rendered label is [redirects_nonExisting]. As described earlier, removing this line should ensure that the rendered label is instead Fallback.

[github-actions]

Hi there @abjerner, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[georgebid]

Thank you for your PR @abjerner, someone on the core collaborators team will give this a review soon 😄

[abjerner]

Hi @bergmania

I can see my PR was closed, but without any further explanation?

Or was the PR just automatically closed when deleting the release/13.0 branch?

[bergmania]

Hi @abjerner, I think this happened automatically. I did not close it intentionally.

[nul800sebastiaan]

Didn't get to it on time for v13.0 unfortunately. Have re-opened for the contrib branch!

[emmagarland]

FYI Have retargeted to v13 as contrib is now for v14.