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 placeholder text translation #38618

Merged
merged 8 commits into from
Feb 3, 2021
Merged

Conversation

hacodeorg
Copy link
Contributor

@hacodeorg hacodeorg commented Jan 18, 2021

FND-1359
The original work at #36983 enabled translation for placeholder texts in Blockly blocks. However, it covers only 1 type of block which is text block. This PR adds support for 2 more types of block: studio_showTitleScreen and studio_ask.

Examples

Example 1: Translating 2 placeholder strings in a studio_showTitleScreen block from English to Vietnamese.

Original Translated
Screen Shot 2021-01-17 at 8 48 50 PM Screen Shot 2021-01-17 at 8 46 15 PM

Example 2: Translating placeholder strings in a studio_ask block ("ask... to set...") and a text block ("...") from English to Vietnamese.

Original Translated
Screen Shot 2021-01-17 at 8 50 15 PM Screen Shot 2021-01-17 at 8 45 49 PM

Testing story

@hacodeorg hacodeorg requested a review from a team as a code owner January 18, 2021 06:01
@hacodeorg hacodeorg requested review from daynew and annaxuphoto and removed request for a team January 18, 2021 06:02
@hacodeorg hacodeorg requested a review from Hamms January 25, 2021 21:21
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

this looks fantastic! Great isolation of functionality

bin/i18n/sync-in.rb Outdated Show resolved Hide resolved
dashboard/app/models/levels/blockly.rb Outdated Show resolved Hide resolved
@hacodeorg hacodeorg force-pushed the ha/fix-placeholder-translation branch from f6cd115 to 0437899 Compare January 27, 2021 16:59
Copy link
Member

@daynew daynew 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 but please check out my comments :)

Comment on lines +548 to 567
<block type="studio_ask">
<title name="TEXT">#{original_str}</title>
</block>
<block type="studio_showTitleScreen">
<title name="TITLE">#{original_str}</title>
<title name="TEXT">#{original_str}</title>
</block>
</xml>
</start_blocks>
</blocks>
</GamelabJr>
XML
localized_block_xml = level.localized_text_blocks(block_xml)
localized_block_xml = level.localized_blocks_with_placeholder_texts(block_xml)

# Expected result is an one-line XML, in which the original string
# has been replaced by a localized string.
block_xml_cleaned = block_xml.strip.gsub(/\s*\n\s*/, '')
expected_localized_block_xml = block_xml_cleaned.sub(original_str, localized_str)
expected_localized_block_xml = block_xml_cleaned.gsub(original_str, localized_str)

assert_equal expected_localized_block_xml, localized_block_xml
Copy link
Member

@daynew daynew Feb 1, 2021

Choose a reason for hiding this comment

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

Does this test that there are 3 instances of original_str? I'm concerned it only detects that there is at least one instance of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, sub function replaces only 1 instance. But gsub will replace all the occurrences of the original_str by localized_str.

# Localize placeholder texts in text blocks
def localized_text_blocks(blocks)
# Localizing placeholder texts in certain block types
def localized_blocks_with_placeholder_texts(blocks)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should add documentation of what a placeholder_text is and maybe a link to a an example block which uses it? We could also just put raw XML in the comments.

I know not many methods have any documentation at all, but it might be a good idea to save the knowledge you gained investigating this task.

I leave it up to you to decide if you think this is worth documenting and also where the right place for the documentation is.

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 have an example of a block with placeholder strings in a test. I can add a comment to let people know that test exists.

@hacodeorg hacodeorg merged commit 9166d01 into staging Feb 3, 2021
@hacodeorg hacodeorg deleted the ha/fix-placeholder-translation branch February 3, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants