oblique-bit / oblique

An Angular front-end framework Tailored for your swiss branded business web application, Oblique provides a standardized corporate design look and feel as well as a collection of ready-to-use Angular components. Oblique, through its fully customizable master layout, takes care of the application's structure, letting you focus on the content.
https://oblique.bit.admin.ch
MIT License
56 stars 13 forks source link

Broken Breadcrumb Component #138

Closed mumenthalers closed 1 month ago

mumenthalers commented 2 months ago

tldr; Breadcrumb Component does beautify theRouteData#breacrumb key prior of (trying) translating it when using path variables/params

Oblique Version: 11.2.0

Description

When defining the Route path with a param AND a breadcrumb key, the breadcrumb key is capitalized and will therefore not be translated. (unless you define the translation key capitalized)

{
  path: `:id`,
  data: { breadcrumb: 'my.key.i.want.to.use' }, // yes, we use lower-camelcase keys.
}

--> will end up as My.key.i.want.to.use

Expected Behaviour

When providing a breadcrumb key for a route, the key should be translated without applying any transformation prior trying to translate.

Context

There are mianly two problems in the code:

  1. the information whether it is a key to translate or a value from the url is lost

https://github.com/oblique-bit/oblique/blob/d9ed7d79042f407b61f355b21039cbde8dc4829a/projects/oblique/src/lib/breadcrumb/breadcrumb.component.ts#L84C4-L84C52

  1. the crumb.label is always tried to translate, no matter what it actually is (also when it is only the path variable itself) --> therefore it will be mistakenly handled with a potential MissingTranslationHandler (ngx-translate)

https://github.com/oblique-bit/oblique/blob/d9ed7d79042f407b61f355b21039cbde8dc4829a/projects/oblique/src/lib/breadcrumb/breadcrumb.component.html#L13C7-L13C21

Proposed Solution

differ between translation key and "no translation key".

dariopog-foitt commented 2 months ago

Hi @mumenthalers,

Thx for reaching out! I will check the issue ASAP and let you know!

dariopog-foitt commented 2 months ago

I did several test but I am not able to reproduce the issue...

I defined my route with:

{
  path: 'test',
  data: {
    breadcrumb: 'my.key.i.want.to.use'
  }
}

and used the <ob-breadcrumb /> in my template. If the my.key.i.want.to.use is not defined, the breadcrumb renders my.key.i.want.to.use. If I define a translation for it, for example

{
  ...,
  "my.key.i.want.to.use": "Hello, World!"
}

the breadcrumb renders the Hello, World! text.

Am I missing something?

mumenthalers commented 2 months ago

hey @dariopog-foitt I'm sorry, I should have provided a repro StackBlitz in the first place. voilà

So what I figured out when creating it:

Still, this behavior is to be considered as bug(s), since it is not documented that all translation keys have to start with i18n (and which in most cases isn't a problem). + the translation interpolation should work like everywhere else

dariopog-foitt commented 2 months ago

Ok, got it! Thx for the clarification, will look into it! Progress tracked with ticket OUI-3289

gillerr commented 2 months ago

Hi, @mumenthalers

Here my take on the 3 problems you reported:

beautifyUrls This option is intended as an alternative to a label. When no label is provided, the URL will be used instead and the beautufyUrl option serves to capitalize the URL parts and transform the hyphens into whitespaces. The beautifyUrl parameter should then only be applied to URL parts and ignore the provided label if any. This, however, is not what the code actually does. We'll fix that.

i18n The idea there was that if your text don't start with i18n, then it's not a translation key and therefore should not be translated. This allows you to provide the same text for all languages. I don't think it's relevant anymore as the translation service will return the key if it cannot be translated. We'll fix that too.

interpolation You're absolutely right, parameters for the breadcrumb labels should use regular interpolation. We'll also fix that.

gillerr commented 1 month ago

This has been fixed with 11.3.4