Skip to content

Conversation

@simi
Copy link
Contributor

@simi simi commented Jun 22, 2025

@potiuk follow up to #52014 (creating related link)

happy to repeat the same for variables (and DRY it a little)

image

PS: Hyperlinks are not super easy to detect with current Chakra UI theme, but it is out of scope of this PR. They looks similar to text and just get underline on hover.

@simi simi force-pushed the conn-not-found-help branch from 66ad6c6 to 0b630bf Compare June 22, 2025 15:49
@simi
Copy link
Contributor Author

simi commented Jun 22, 2025

updated @jscheffl

image

@simi simi force-pushed the conn-not-found-help branch from 0b630bf to 8690776 Compare June 22, 2025 16:02
@simi
Copy link
Contributor Author

simi commented Jun 22, 2025

🇩🇪 example

image

@simi simi force-pushed the conn-not-found-help branch from 8690776 to 1bf1da2 Compare June 22, 2025 16:03
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the extension (and small rework because of my translation nit)

@simi
Copy link
Contributor Author

simi commented Jun 22, 2025

@jscheffl was fun. 🤝

@jscheffl
Copy link
Contributor

@jscheffl was fun. 🤝

Keep it coming :-D (I mean you said you'd do the same for Variables...)

@simi
Copy link
Contributor Author

simi commented Jun 22, 2025

@jscheffl should I continue here?

@jscheffl
Copy link
Contributor

@jscheffl should I continue here?

I'd propose a separate PR.

Oh, some static checks fail here. Probably you can install pre-commit locally then you see it directly before CI throws an error...

@potiuk
Copy link
Member

potiuk commented Jun 22, 2025

Oh, some static checks fail here. Probably you can install pre-commit locally then you see it directly before CI throws an error...

Yep. pre-commit install is cool

@simi simi force-pushed the conn-not-found-help branch from 1bf1da2 to 9f83895 Compare June 23, 2025 05:51
@simi
Copy link
Contributor Author

simi commented Jun 23, 2025

@jscheffl @potiuk I'm not big fan of pre-commit install, but I was able to check and fix on troubles (running individual ones locally to reproduce). Should be ok now.

@potiuk
Copy link
Member

potiuk commented Jun 23, 2025

@jscheffl @potiuk I'm not big fan of pre-commit install, but I was able to check and fix on troubles (running individual ones locally to reproduce). Should be ok now.

Understood (though i am a big fan!). Just FYI breeze static-checks --only-my-changes is manual equivalent of running all static checks without having pre-commit install

@potiuk potiuk merged commit 9ff9cc4 into apache:main Jun 23, 2025
50 checks passed
@simi simi deleted the conn-not-found-help branch June 23, 2025 06:41
@simi
Copy link
Contributor Author

simi commented Jun 23, 2025

@potiuk I'm not big fan of breeze (yet) also since it is container based and much slower to "native" experience. I actually use pre-commit, but not with install to control when to run what. And locally after failed CI I'm running commands like pre-commit run ts-compile-lint-ui -s HEAD~1 -o HEAD to check on last commit which seems to run basic pnpm commands like pnpm run format and pnpm run lint which are closer to native experience if run manually. Anyway thanks for the tips, I'm slowly getting into.

@potiuk
Copy link
Member

potiuk commented Jun 23, 2025

Indeed pre-commit run ts-compile-lint-ui -s HEAD~1 -o HEAD also works yes.

And actually breeze does not use containers to run static-checks.

Mostly what it does is to map the "nicer" options we have (like --only-my-chages) into the right pre-commit command (you might see yourself by running breeze static-check --only-my-changes --dry-run) - also what it has is auto-complete of the pre-commits (so you could type breeze static-checks --type ts-<TAB> if you do not remember the pre-commit name. --only-my-changes is also nice when you have multiple commits - as it automatically finds the right commit you are based on - and runs pre-commit for all commits between. You can also do it with the right git *smth* command but I never remember that one :).

And pre-commit itself automatically selects which pre-commit to run based on your changes, so usually there is even no need to specify which pre-commit to run.

BTW. I tried to contribute some of those changes (auto-complete and nice shortcuts for --last-commit and --only-my-changes to pre-commit, but it was rejected, so it only works in breeze static-checks..

@simi
Copy link
Contributor Author

simi commented Jun 23, 2025

@potiuk noted, thanks, I'll give it another try.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, nice job.

Just for future work @potiuk @jscheffl I don't think we should accept PRs that update all translation at the same time. Most likely because this can (and will) introduce translation approximations unless translation owners for each language approves the PR (which will delay it quite a lot). (for instance in French you need a space before ! and ?, that's not ultra important, but that could have been a plain wrong translation, same for other languages).

@simi
Copy link
Contributor Author

simi commented Jun 23, 2025

@pierrejeambrun I did my best to scan locales around for proper punctuation style (and I have detected, some of them are not using at all and skipped them on purpose), but I have missed that in French (sorry, I'll fix in PR), since I have seen . few lines above and had no idea ! works differently.

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Jun 23, 2025

No worries @simi, that is not your fault, people contributing PR can't be proficient in so many languages and this is why I am suggesting slightly different approach so you don't have to actually make the translation for all of them (using AI tools or translation tools without the ability to actually review the outcome) when you try to contribute a new feature to the UI. (There is already a fallback to english to give translation owners time to contribute the additional missing keys.)

@potiuk
Copy link
Member

potiuk commented Jun 23, 2025

Just for future work @potiuk @jscheffl I don't think we should accept PRs that update all translation at the same time. Most likely because this can (and will) introduce translation approximations unless translation owners for each language approves the PR (which will delay it quite a lot). (for instance in French you need a space before ! and ?, that's not ultra important, but that could have been a plain wrong translation, same for other languages).

But this one did not add any translations - it only moved the translations when the key was moved. We still have to translate the added strings :)

@potiuk
Copy link
Member

potiuk commented Jun 23, 2025

I think moving keys is "OK" to do in translations at the same time

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Jun 23, 2025

Ah! This one is tricky, my bad you are right, I reviewed it too fast and assumed it was new added keys, while in fact it's updating a key (the punctuation) which ended up not accurate in french.

@pierrejeambrun
Copy link
Member

I update my recommendation:

  • Moving keys is fine
  • We shouldn't add keys for non default (en) translations
  • We shoudn't update keys for non default (en) translation

@potiuk
Copy link
Member

potiuk commented Jun 23, 2025

I update my recommendation:

  • Moving keys is fine
  • We shouldn't add keys for non default (en) translations
  • We shoudn't update keys for non default (en) translation

💯

@potiuk
Copy link
Member

potiuk commented Jun 23, 2025

Ah! This one is tricky, my bad you are right, I reviewed it too fast and assumed it was new added keys, while in fact it's updating a key (the punctuation) which ended up not accurate in french.

I actually had very same impression when I saw it and I wanted to make exactly the same comment. But then I looked closer :D

@jscheffl
Copy link
Contributor

Ah! This one is tricky, my bad you are right, I reviewed it too fast and assumed it was new added keys, while in fact it's updating a key (the punctuation) which ended up not accurate in french.

I actually had very same impression when I saw it and I wanted to make exactly the same comment. But then I looked closer :D

...and to contribute to the discussion I requested @simi to move the keys for all languages :-D with the request not to change the text. So 100% with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:translations area:UI Related to UI/UX. For Frontend Developers. translation:default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants