[DO NOT MERGE] changes to accept i18Nlabels as input #397

RAJESH-GN · 2025-01-29T16:13:50Z

No description provided.

tcorral · 2025-01-31T08:27:28Z

@RAJESH-GN Fix the build, please.

RAJESH-GN · 2025-01-31T08:49:49Z

@tcorral fixed the tests. Can u please validate it once u have some time.

tcorral

We have to discuss it further as I see many different things that we should align across implementations.

tcorral · 2025-01-31T12:21:41Z

apps/golden-sample-app/src/app/app.component.html

    >
-      Skip to Content
+      {{ this.translations['app.skipToContent'] }}


I don't think the 'this.' is needed here as it's not needed in line '16'.

Where the 'app.skipToContent' comes from? I have checked out the branch and I cannot find it, just the usage?
Also in my case I kept the original custom id and in this change it's changed to use camel case instead of using snake-case? Can you get it aligned, please?

Suggested change

{{ this.translations['app.skipToContent'] }}

{{ this.translations['skip-to-content'] }}

thanks was my bad

tcorral · 2025-01-31T12:26:42Z

apps/golden-sample-app/src/app/app.component.html

-        accessibility feature to quickly navigate to the main content section.
-        This aria-label is located in the topbar area of the
-        layout.@@move-focus-to-beginning-of-content"
+      [attr.aria-label]="translations['move-focus-to-beginning-of-content']"


After the discussion on my PR about using [attr.aria-label] I investigated a bit more and in this case using [ariaLabel] is allowed because this property actually exist in the button tag and Angular transforms it properly to 'aria-label' property in the HTML. Can you get it aligned, please?

Suggested change

[attr.aria-label]="translations['move-focus-to-beginning-of-content']"

[ariaLabel]="translations['move-focus-to-beginning-of-content']"

tcorral · 2025-01-31T12:27:43Z

apps/golden-sample-app/src/app/app.component.html

-        as an accessibility feature to indicate the button's function. This
-        aria-label is located in the topbar area of the
-        layout.@@bb-layout.sidebar_toggler"
+      [attr.aria-label]="translations['bb-layout.sidebar_toggler']"


After the discussion on my PR about using [attr.aria-label] I investigated a bit more and in this case using [ariaLabel] is allowed because this property actually exist in the button tag and Angular transforms it properly to 'aria-label' property in the HTML. Can you get it aligned, please?

Suggested change

[attr.aria-label]="translations['bb-layout.sidebar_toggler']"

[ariaLabel]="translations['bb-layout.sidebar_toggler']"

tcorral · 2025-01-31T12:28:25Z

apps/golden-sample-app/src/app/app.component.html

-        as an accessibility feature to indicate the button's function. This
-        aria-label is located in the topbar area of the
-        layout.@@bb-layout.sidebar_toggler"
+      [attr.aria-label]="translations['bb-layout.sidebar_toggler']"
      [attr.aria-expanded]="layoutService.navigationExpanded$ | async"


After the discussion on my PR about using [attr.aria-label] I investigated a bit more and in this case using [ariaExpanded] is allowed because this property actually exist in the button tag and Angular transforms it properly to 'aria-label' property in the HTML. Can you get it aligned, please?

Suggested change

[attr.aria-expanded]="layoutService.navigationExpanded$ | async"

[ariaExpanded]="layoutService.navigationExpanded$ | async"

done thomas thanks

tcorral · 2025-01-31T12:34:10Z

apps/golden-sample-app/src/app/app.component.ts

@@ -8,6 +8,10 @@ import {
 } from '@backbase/foundation-ang/observability';
 import { environment } from '../environments/environment';

+export interface Translations {


In my code and after a few iterations I have found that having a catalog with the translations might be more easy to debug and find changes but I also noticed that using Translations every where will be a mess because in order for the developers to overwrite the translations for each journey or component we will need to export the translations and it has more sense to have it's own name like I have in the suggestion.

Suggested change

export interface Translations {

export interface AppTranslations {

tcorral · 2025-01-31T13:35:09Z

...al/feature/src/lib/components/ach-positive-pay-journey/ach-positive-pay-journey.component.ts

 import { ActivatedRoute, Router, RouterModule } from '@angular/router';
 import { PERMISSIONS } from '@backbase-gsa/ach-positive-pay-journey/internal/shared-data';
 import { HeadingModule } from '@backbase/ui-ang/heading';

+export interface Translations {
+  [key: string]: string;


Same reason that for my previous similar comments.

Suggested change

[key: string]: string;

'ach-positive-pay-journey.heading.title': string;

'ach-positive-pay-journey.heading.new-blocker.button': string;

[key: string]: string;

applied the change

tcorral · 2025-01-31T13:35:39Z

...al/feature/src/lib/components/ach-positive-pay-journey/ach-positive-pay-journey.component.ts

@@ -11,11 +15,31 @@ import { HeadingModule } from '@backbase/ui-ang/heading';
 })
 export class AchPositivePayJourneyComponent {
  permissions = PERMISSIONS;
+  @Input()
+  public readonly translations: Translations = {};


Suggested change

public readonly translations: Translations = {};

public readonly translations: AchPositivePayJourneyTranslations = {};

tcorral · 2025-01-31T13:35:55Z

...al/feature/src/lib/components/ach-positive-pay-journey/ach-positive-pay-journey.component.ts

+  @Input()
+  public readonly translations: Translations = {};
+
+  public readonly defaultTranslations: Translations = {


Suggested change

public readonly defaultTranslations: Translations = {

public readonly defaultTranslations: AchPositivePayJourneyTranslations = {

tcorral · 2025-01-31T13:37:31Z

apps/golden-sample-app/src/app/app.component.ts

@@ -17,6 +21,74 @@ export class AppComponent {
  triplets = triplets;
  isAuthenticated = false;

+  public readonly translations: Translations = {


As we don't know if the user would like to override one or more or all of them we have to use Partial<>

Suggested change

public readonly translations: Translations = {

public readonly translations: Partial<AchPositivePayJourneyTranslations> = {

we dont need to use partials now i have made a new commit to address this issue please check

tcorral · 2025-01-31T13:38:55Z

apps/golden-sample-app/src/app/user-accounts/user-accounts-view/user-accounts-view.component.ts

+  ) {
+    this.translations = {
+      ...this.defaultTranslations,
+      ...this.translations,


I am not sure if this might work after using Partial<> after setting a more strict interface can you test?

Sure tomas i will try different approach

tcorral · 2025-02-07T10:52:59Z

apps/golden-sample-app/src/app/app.component.html

    >
-      Skip to Content
+      {{ appTranslations['skipToContent'] }}
    </button>

    <!-- Hamburger -->
    <button


It's a problem with this button because on running the accesibility linter it asks to have a text for the button. In my case I added the following code after the line 29.

<span class="sr-only">{{ translations['bb-layout.sidebar_toggler'] }}</span>

In your code it should be something like:

<span class="sr-only">{{ appTranslations['bb-layout.sidebar_toggler'] }}</span>

For this to work you also will need to add a class on the scss file:

.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0, 0, 0, 0); white-space: nowrap; border-width: 0; }

If you don't do this, the e2e tests will fail

changes to accept i18Nlabels as input

293a3de

RAJESH-GN requested a review from tcorral January 29, 2025 16:13

RAJESH-GN added 2 commits January 29, 2025 17:30

changes to accept i18Nlabels as input

243916a

changes to accept i18Nlabels as input

608f748

tests updated

36ee772

tcorral reviewed Jan 31, 2025

View reviewed changes

RAJESH-GN added 5 commits February 3, 2025 09:32

comments on the input changes

fb4be66

merging received Inputs

18ec5e0

merging received Inputs routing example

b386ebb

merging received Inputs routing example bug fix

df5f969

merging received Inputs routing example bug fix

763f9b2

tcorral reviewed Feb 7, 2025

View reviewed changes

changes to variable referenced

3616818

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] changes to accept i18Nlabels as input #397

[DO NOT MERGE] changes to accept i18Nlabels as input #397

RAJESH-GN commented Jan 29, 2025

tcorral commented Jan 31, 2025 •

edited

Loading

RAJESH-GN commented Jan 31, 2025 •

edited

Loading

tcorral left a comment

tcorral Jan 31, 2025

tcorral Jan 31, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 3, 2025

RAJESH-GN Feb 3, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 3, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 3, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 3, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 3, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 4, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 4, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 4, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 4, 2025

tcorral Jan 31, 2025

RAJESH-GN Feb 3, 2025

tcorral Feb 7, 2025

	{{ this.translations['app.skipToContent'] }}
	{{ this.translations['skip-to-content'] }}

	[attr.aria-label]="translations['move-focus-to-beginning-of-content']"
	[ariaLabel]="translations['move-focus-to-beginning-of-content']"

	[attr.aria-label]="translations['bb-layout.sidebar_toggler']"
	[ariaLabel]="translations['bb-layout.sidebar_toggler']"

	[attr.aria-expanded]="layoutService.navigationExpanded$ \| async"
	[ariaExpanded]="layoutService.navigationExpanded$ \| async"

	export interface Translations {
	export interface AppTranslations {

	public readonly translations: Translations = {};
	public readonly translations: AchPositivePayJourneyTranslations = {};

	public readonly defaultTranslations: Translations = {
	public readonly defaultTranslations: AchPositivePayJourneyTranslations = {

	public readonly translations: Translations = {
	public readonly translations: Partial<AchPositivePayJourneyTranslations> = {

[DO NOT MERGE] changes to accept i18Nlabels as input #397

Are you sure you want to change the base?

[DO NOT MERGE] changes to accept i18Nlabels as input #397

Conversation

RAJESH-GN commented Jan 29, 2025

tcorral commented Jan 31, 2025 • edited Loading

RAJESH-GN commented Jan 31, 2025 • edited Loading

tcorral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcorral commented Jan 31, 2025 •

edited

Loading

RAJESH-GN commented Jan 31, 2025 •

edited

Loading