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

Fix translation extraction #1776

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Fix translation extraction #1776

merged 1 commit into from
Mar 29, 2021

Conversation

jotoeri
Copy link
Contributor

@jotoeri jotoeri commented Mar 22, 2021

Two small things:

  • As plural-translations have a format of n('%n Action', '%n Actions', n) the extraction needs to use parameters 0 and 1. Didn't get recognized, as there are currently no plural-translations on this repo.
  • Currently, translations do not get extracted if they are directly written within a html-property. Normally that should be fixed on extraction, however, the author seems to be on a bigger rewrite (since two years? Can't extract from angular template component input attributes lukasgeiter/gettext-extractor#31 🤔), so this is a quick and simple workaround to put them on new lines.

@jotoeri jotoeri added 3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. technical debt labels Mar 22, 2021
Comment on lines 134 to 135
v-tooltip.auto="
t('Close')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird, and most likely invalid with our styleguide 🤔

Copy link
Contributor Author

@jotoeri jotoeri Mar 22, 2021

Choose a reason for hiding this comment

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

Yeah, however linter does not complain, i checked that as i was also expecting some problems... 🤔

Alternative approach would be this, it's just a bit more of code...

data {
  return {
    closeTranslation: t('Close')
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need the newline anyway?

Copy link
Contributor Author

@jotoeri jotoeri Mar 22, 2021

Choose a reason for hiding this comment

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

npm run l10n:extract respectively gettext-extractor (build/extract-l10n.js) did not recognise it within an html-tag. So no extraction, no translations on transifex... 😢

Here e.g. the close-button is not translated:
grafik

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we fix the gettext-extractor directly? No one is going to remember putting new lines :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find a way... A PR on the repo seems not worth the work, as there are other PRs that just didn't get merged (see linked issue above and this PR).
Only other way, that i could imagine would be to use a html-parser with specific html-tags t and n to put the translations in a second time, but thats even more hacky.
Or just some completely different parser-package... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any preferences? As i said - i don't see us currently fixing the extractor.

  • Using the given way or the proposed data/prop way? The latter is probably a bit cleaner, but 5 LoC more. The prop could also be done as object l10n.close = t('Close')
  • Just came to my mind that i could additionally add a workflow spitting a warning if some pattern ="t( is found. This is how i found those three occurences.

Copy link
Contributor

@skjnldsv skjnldsv Mar 25, 2021

Choose a reason for hiding this comment

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

data {
return {
closeTranslation: t('Close')
}
}

Let's use this syntax instead then, will be cleaner and easier to manage

The other option I could think of would be to build the dev webpack and parse the results

Copy link
Contributor Author

@jotoeri jotoeri Mar 28, 2021

Choose a reason for hiding this comment

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

Ok, so it's this version now.

The webpack-dev-build i looked into for moment, but it seems to be not fully straight forward.

@jotoeri jotoeri added 2. developing Work in progress 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 2. developing Work in progress labels Mar 22, 2021
@jotoeri jotoeri force-pushed the fix/translation_extract branch from 43ecfc2 to ec733aa Compare March 27, 2021 01:01
@jotoeri jotoeri added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 27, 2021
Signed-off-by: Jonas Rittershofer <[email protected]>
@jotoeri jotoeri force-pushed the fix/translation_extract branch from ec733aa to 50e88a1 Compare March 28, 2021 21:39
@jotoeri jotoeri added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 29, 2021
@jotoeri jotoeri merged commit 025aabf into master Mar 29, 2021
@jotoeri jotoeri deleted the fix/translation_extract branch March 29, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants