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

Development: Change translations to follow new guidelines #8139

Merged

Conversation

rstief
Copy link
Contributor

@rstief rstief commented Mar 5, 2024

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.

Motivation and Context

Changes all tags with jhiTranslate to conform to the new client coding guidelines specified in #8132
This also reduces the bundle size.

Description

Replaces all occurrences of text between tags with jhiTranslate using the following regex:
(<([\w-]+)[^>]* jhiTranslate=".+"[^>]*>)[^{<]*(<\/\2>)

Replacing occurrences of artemisTranslate has been omitted, since it might lead to failing tests and more inconsistencies in the code. Simple occurrences can be replaced by using the following regexes:
(<.+)>\{\{ '([\w\d.-]*)' \| artemisTranslate \}\}(<\/.+>) (to find)
$1 jhiTranslate="$2">$3 (to replace)

Steps for Testing

Run Artemis and verify no translations break

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked







Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Test Coverage

unchanged

@artemis-bot artemis-bot added this to In progress in Artemis Development Mar 5, 2024
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Mar 5, 2024
@rstief rstief temporarily deployed to artemis-test4.artemis.cit.tum.de March 5, 2024 11:39 — with GitHub Actions Inactive
@farisd16 farisd16 temporarily deployed to artemis-test2.artemis.cit.tum.de March 5, 2024 14:19 — with GitHub Actions Inactive
Copy link
Member

@farisd16 farisd16 left a comment

Choose a reason for hiding this comment

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

tested on ts2, everything seems fine

farisd16
farisd16 previously approved these changes Mar 5, 2024
Copy link
Member

@farisd16 farisd16 left a comment

Choose a reason for hiding this comment

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

tested on ts2, everything seems fine

Artemis Development automation moved this from In progress to Review in progress Mar 11, 2024
@artemis-bot artemis-bot moved this from Review in progress to In progress in Artemis Development Mar 11, 2024
laurenzfb
laurenzfb previously approved these changes Mar 11, 2024
Copy link
Contributor

@laurenzfb laurenzfb left a comment

Choose a reason for hiding this comment

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

looks good. regex makes sense

@rstief rstief marked this pull request as ready for review March 11, 2024 14:49
@rstief rstief requested a review from a team as a code owner March 11, 2024 14:49
@artemis-bot artemis-bot moved this from In progress to Ready for review in Artemis Development Mar 11, 2024
milljoniaer
milljoniaer previously approved these changes Mar 11, 2024
Copy link
Contributor

@milljoniaer milljoniaer left a comment

Choose a reason for hiding this comment

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

Looked through the changes, LGTM 🚀

RY997
RY997 previously approved these changes Mar 11, 2024
Copy link
Contributor

@RY997 RY997 left a comment

Choose a reason for hiding this comment

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

Went through the changes. Looks good.

@MaximilianAnzinger MaximilianAnzinger added maintainer-approved The feature maintainer has approved the PR ready to merge and removed ready for review labels Mar 11, 2024
Copy link
Contributor

@MaximilianAnzinger MaximilianAnzinger left a comment

Choose a reason for hiding this comment

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

Regex & Code

Copy link

⚠️ Unable to deploy to test servers ⚠️

The docker build needs to run through before deploying.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Mar 11, 2024
@rstief rstief added deploy:artemis-test1 and removed deployment-error Added by deployment workflows if an error occured labels Mar 11, 2024
@rstief rstief temporarily deployed to artemis-test1.artemis.cit.tum.de March 11, 2024 17:50 — with GitHub Actions Inactive
Artemis Development automation moved this from Ready for review to Review in progress Mar 11, 2024
@github-actions github-actions bot added the tests label Mar 11, 2024
Copy link
Contributor

@RY997 RY997 left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Reapprove!

@krusche krusche merged commit d0851e6 into develop Mar 11, 2024
26 of 30 checks passed
@krusche krusche deleted the chore/development/make-translations-follow-new-guidelines branch March 11, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) maintainer-approved The feature maintainer has approved the PR tests
Projects
No open projects
Artemis Development
  
Review in progress
Development

Successfully merging this pull request may close these issues.

None yet

8 participants