Skip to content

feat: adds localization to uui-pagination #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: v1/contrib
Choose a base branch
from

Conversation

NguyenThuyLan
Copy link
Contributor

Description

The uui-pagination element has no options for localization.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

I added 4 labels for 4 buttons (First, Previous, Next, Last) to generate their names.

How to test?

Screenshots (if appropriate)

image

@iOvergaard iOvergaard changed the title Fix localization of uui-pagination feat: adds localization to uui-pagination Apr 11, 2025
@iOvergaard iOvergaard added the enhancement New feature or request label Apr 11, 2025
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1069.westeurope.azurestaticapps.net

Copy link
Contributor

@iOvergaard iOvergaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great so far. I see that we have hardcoded texts for "More pages" and "Current page: n" as well as "Go to page n". We need to localize those as well.

Maybe we could think of some way to group all of these, since it will be a lot of properties. How about using a record? Something like:

  @property({ type: Object })
  labels = {
    first: 'First',
    previous: 'Previous',
    next: 'Next',
    last: 'Last',
    current: 'Current page: {0}',
    goTo: 'Go to page {0}',
    showMore: 'More pages',
  };

Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1069.westeurope.azurestaticapps.net

@iOvergaard
Copy link
Contributor

iOvergaard commented Apr 11, 2025

Great so far. I see that we have hardcoded texts for "More pages" and "Current page: n" as well as "Go to page n". We need to localize those as well.

Maybe we could think of some way to group all of these, since it will be a lot of properties. How about using a record? Something like:

  @property({ type: Object })
  labels = {
    first: 'First',
    previous: 'Previous',
    next: 'Next',
    last: 'Last',
    current: 'Current page: {0}',
    goTo: 'Go to page {0}',
    showMore: 'More pages',
  };

I spoke with @nielslyngsoe, and we think it could make sense to introduce a factory function property to translate these things including variables. The profile of such a function could be:

@property({ attribute: false })
localize: (type: 'first' | 'last' | 'current' | 'xxx', ...args: unknown[]) => string = (type, ...args) => {
  switch(type) {
    case 'first':
      return 'First';
    case 'current':
      return 'Current page: ' + args[0];
  }
}

It would be its job to find a suiting translation for the type in question and could be called from the render() function directly:

render() {
  return html`<uui-button label=${this.localize('current', this.currentPage)></uui-button>`;
}

What do you think of that?

@nielslyngsoe
Copy link
Member

Hi @NguyenThuyLan In the perspective of how we code-style wise have approached labels and messages previously I do have a different suggestion, which is building on top of the existing architecture where we have a property/attribute for each.
The style of what you have already done in this PR.

To support arguments for those labels, my suggestion is to enable the message properties to additionally accept a method. In this style:

firstLabel: string | () => string = 'First';

For one that accepts an argument it would look like this:

currentLabel: string | (current: number) => string = (n) => `Current page ${n}`;

So if you would like to bring your localization for this, it can look like this:

html`<uui-button .currentLabel=${(n) => this.localize('madeUpKeyForThisExample_currentPage', n)></uui-button>`;

In this way each label property can bring the relevant arguments for the use case, without poluting a general localization method.

And the implementor can to use strings for the labels where no arguments are needed:

html`
<uui-button
    firstLabel="First"
    .currentLabel=${(n) => this.localize('madeUpKeyForThisExample_currentPage', n)
    ></uui-button>
`;

This is important cause we have to remember the Backoffice is not the only usage of the UI Library. We have the Cloud Portal which only exists in English, so we should not make their life more complicated. And as well there are all the extensions, which again most extensions do not have localized labels.

I hope it makes sense, feel free to get in contact if you like to evaluate

Thanks

@nielslyngsoe
Copy link
Member

nielslyngsoe commented Apr 14, 2025

Additional information regarding the implementation, regarding the addValidator from another PR:

Just wanted to make it available that the addvaidator implementation would look like this:

this.addValidator(
      'tooLong',
      () => {
        const label = this.maxlength;
        if (typeof label === 'function') {
            return label(this.maxlength, String(this.value).length);
        }
        return label;
      }
      () => !!this.maxlength && String(this.value).length > this.maxlength,
    );
    ```

Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1069.westeurope.azurestaticapps.net

@@ -214,11 +245,9 @@ export class UUIPaginationElement extends LitElement {
look="outline"
class="nav"
role="listitem"
label="Go to first page"
label=${this.firstLabel}
Copy link
Member

@nielslyngsoe nielslyngsoe Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the label is a method then this will not work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see I have caused some confusion here. Cause the general conversation around localization was from my side focusing on the ability to bring arguments to the labels. Looking at these specific four labels, then I only think it makes sense to accept a method when there is arguments to be considered for the label.

These four does not use any arguments as it stands right now. So I think they can just be of type string and then this is all fine :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants