Skip to content
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

Custom field translations #18145

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

cconard96
Copy link
Contributor

@cconard96 cconard96 commented Oct 28, 2024

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

Allow translating custom field labels

@cconard96 cconard96 added the wip label Oct 28, 2024
@cconard96 cconard96 force-pushed the customasset/field_trans branch from f0c1566 to a26e556 Compare November 1, 2024 19:46
@cconard96
Copy link
Contributor Author

Waiting for functional/technical review to ensure this feature is finalized before adding tests.

@cconard96 cconard96 marked this pull request as ready for review November 2, 2024 10:16
@orthagh
Copy link
Contributor

orthagh commented Nov 4, 2024

Ok, I was curious about where the translation system will be displayed.
Re-using the existing tab (translations) is clever in fact.

I suggest the following changes:

  • remove the "Custom field translation"
  • Always display the "Field" dropdown
  • include the "asset name" option in this dropdown
  • add a helper on the top of the modal explaining you can translate either the asset name or custom fields.
  • merge the 2 tables into the second one, translation entry for the asset name displayed in the single list, and differentiate again by filling the "Field" column.

There is not so much work remaining (I don't know about tests) and this pr could be integrated very soon.

@cconard96 cconard96 force-pushed the customasset/field_trans branch from d777484 to 72533d0 Compare November 19, 2024 12:09
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

Apart from the few minor comments I made, it seems to me to be OK from the technical point of view.

A more generic system for the translation storage may be introduced by #18223 , but migrating to it, if it is relevant, could be done later and should not be consider as a blocker for the current PR.

src/Glpi/Asset/CustomFieldDefinition.php Outdated Show resolved Hide resolved
src/Glpi/CustomObject/AbstractDefinition.php Outdated Show resolved Hide resolved
src/Glpi/CustomObject/AbstractDefinition.php Outdated Show resolved Hide resolved
@cedric-anne cedric-anne requested a review from orthagh November 21, 2024 13:53
Copy link
Contributor

@orthagh orthagh left a comment

Choose a reason for hiding this comment

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

It's still ok for me from a technical pov.
what do you prefer to merge first @cconard96: this pr or the one for reordering fields ?

@cconard96
Copy link
Contributor Author

It's still ok for me from a technical pov. what do you prefer to merge first @cconard96: this pr or the one for reordering fields ?

I don't think it matters. This one will probably be approved before the custom field reordering one.

@cconard96 cconard96 force-pushed the customasset/field_trans branch from fe64d6d to 25cd9a2 Compare November 25, 2024 12:58
@orthagh orthagh mentioned this pull request Nov 26, 2024
4 tasks
@trasher
Copy link
Contributor

trasher commented Nov 26, 2024

I guess tests should be added, at least for new PHP classes/methods.

@trasher
Copy link
Contributor

trasher commented Nov 26, 2024

Is this one still WIP? (if not, please remove label)

@cconard96 cconard96 force-pushed the customasset/field_trans branch from 2b2dde7 to 93980f9 Compare November 26, 2024 18:10
@cconard96 cconard96 removed the wip label Nov 27, 2024
@cedric-anne cedric-anne added this to the 11.0.0 milestone Nov 28, 2024
@cedric-anne cedric-anne merged commit cab1831 into glpi-project:main Dec 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants