-
Notifications
You must be signed in to change notification settings - Fork 3k
[69972] Inconsistent widget name for "project description" widget on project home page #21539
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
[69972] Inconsistent widget name for "project description" widget on project home page #21539
Conversation
|
|
||
| def title | ||
| Project.human_attribute_name(:status_code) | ||
| I18n.t("attributes.status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| I18n.t("attributes.status") | |
| Project.human_attribute_name(:status) |
does the same thing, but will also error if the translation is missing for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of using that over the I18n variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the same thing, but will also error if the translation is missing for some reason.
Sorry! My earlier statement was inaccurate.
I18n.t("attributes.status") will also raise an error if I18n is configured to do so (for example, in test mode).
That said, model_name.human_attribute_name(:attribute) is still preferable, in my opinion, because it applies intelligent fallback resolution. Specifically, it resolves translations in the following order:
- en.activerecord.attributes.project.status
- en.attributes.status
- en.activerecord.models.status
This means it will fall back to the global status translation by default, while still allowing model-specific overrides. For example, if we later decide that “status” in the context of Project should be renamed to something more specific (e.g., “state” or “condition”), this approach supports that cleanly without affecting other models.
myabc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your changes work just fine. However I'm not sure we need to override widgetName in this way.
I started working on this before the Christmas break but stopped because of another blocking issue (OP # 69921 - now fixed).
I would suggest we just update the title in the translations (js-en.yml):
This would of course also update the text in the "Add widget" dialog, but I think that's probably ok (and perhaps even desirable)?
I guess could also define a separate name translation for widgets.service.ts, but I don't 100% understand its purpose
| } | ||
|
|
||
| public get widgetName() { | ||
| return this.i18n.t('js.label_description'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important to use translations rather than generic translations. A hypothetical example: although description might usually translate to "Beschreibung" in German, in the context of a Project / Project Widget, we might prefer to use the terms "Bezeichnung" or "Langtext".
This is the same reasoning human_attribute_name is preferable. https://github.com/opf/openproject/pull/21539/changes#r2664928430
Hope that makes sense!
|
FYI, #21555 touches the same code - we may want to hold off for that PR to be merged. |
…for programs and portfolios
efd51e5 to
107d2cf
Compare
myabc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Ticket
https://community.openproject.org/wp/69972
What are you trying to accomplish?
Omit the word "project" from the widget titles since they also apply for programs and portfolios