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.

Sign up for GitHub

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

Jump to bottom

Add support for the translationsOutputFile option allowing specifying target bundle for translations #662

Merged
merged 14 commits into from
Aug 3, 2020
Merged

Add support for the translationsOutputFile option allowing specifying target bundle for translations #662

merged 14 commits into from
Aug 3, 2020

Conversation

ma2ciek
Copy link
Contributor

@ma2ciek ma2ciek commented Jul 30, 2020 •
edited
Loading

Suggested merge commit message (convention)

Feature: Add support for the translationsOutputFile option for CKEditorWebpackPlugin allowing specifying the target bundle for translations. Closes ckeditor/ckeditor5#7688.

Additional information

@ma2ciek ma2ciek changed the title WIP Add support for the translationsOutputFile option allowing specifying target bundle for translations Add support for the translationsOutputFile option allowing specifying target bundle for translations Jul 31, 2020
@ma2ciek ma2ciek requested a review from pomek July 31, 2020 09:00
@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jul 31, 2020

@pomek, could you test it on the Vue integration by following the steps from https://github.com/ckeditor/ckeditor5/pull/7755/files#diff-0acf7b6d2c9b5050039b3d5741919a61? I mean both modes (yarn serve and yarn build)

@coveralls
Copy link

coveralls commented Jul 31, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 87.241% when pulling 0ec55b2 on cke5/7688 into 71a1637 on master.

pomek
pomek requested changes Aug 3, 2020
View reviewed changes
Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

I need to figure out how to reproduce the an that the PR should fix. However, cosmetic changes could be applied.

packages/ckeditor5-dev-utils/lib/translations/multiplelanguagetranslationservice.js Outdated
/**
* @param {Object} options
* @param {String} options.outputDirectory Output directory for the translation files relative to the output.
* @param {String[]} options.compilationAssetNames Original asset names from the compiler (e.g. Webpack).
Copy link
Member

Choose a reason for hiding this comment

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

s/String[]/Array.<String>

Copy link
Member

Choose a reason for hiding this comment

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

Missing the @returns tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we document the return everywhere. Especially for such places that the code is rather internal. I can specify only the type as the function name suggests what it returns.

Copy link
Member

Choose a reason for hiding this comment

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

If it returns Object, just put @returns {Object}. We don't need digging into its details.

packages/ckeditor5-dev-utils/lib/translations/multiplelanguagetranslationservice.js Outdated
Comment on lines 392 to 397
/**
*
* @param {String|((name: string) => boolean)|RegExp} predicate
* @param {String[]} options
* @returns {String|undefined}
*/
Copy link
Member

Choose a reason for hiding this comment

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

String|RegExp|Function would be enough. OTOH, why do we need to support such complex construction?

Copy link
Member

Choose a reason for hiding this comment

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

s/String[]/Array.<String>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to support at least strings and regexps. We need strings for specifying the name that might not exist and regexp for matching the existing name. I added support for function as it always allows for full customization.

Copy link
Member

Choose a reason for hiding this comment

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

I would put the comment in the code. In the future, it might help when we will be wondering why it's done this way and what we wanted to resolve.

ma2ciek and others added 2 commits August 3, 2020 11:38
@ma2ciek @pomek
Update packages/ckeditor5-dev-utils/lib/translations/multiplelanguage…
6ed923f 
…translationservice.js

Co-authored-by: Kamil Piechaczek <pomek@users.noreply.github.com>
@ma2ciek
Improved error message.
0ec55b2
@pomek pomek merged commit f385e09 into master Aug 3, 2020
@pomek pomek deleted the cke5/7688 branch August 3, 2020 09:53
@pomek pomek mentioned this pull request Aug 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pomek pomek pomek requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow picking the bundle to which all translations will be outputted
3 participants
@ma2ciek @coveralls @pomek