Improve i18n for 'Copy to backpack' option in multiselect

[ewpatton]

Change-Id: I3bf48a801ba9d503692ebdf9450506f9493cc082

[ewpatton]

This is a small PR to improve the "Copy to backpack" menu item when multiple blocks are selected. This also allows downstream projects to internationalize the message in a way that makes sense for their projects rather than just sticking the number of blocks on the front.

[ewpatton]

I've defined COPY_X_TO_BACKPACK in our sources as "Add %1 Blocks to Backpack", which I think is more readable than just appending a number to the end. If this message is not defined, then the behavior is the same as the backpack plugin.

[HollowMan6]

I've defined COPY_X_TO_BACKPACK in our sources as "Add %1 Blocks to Backpack", which I think is more readable than just appending a number to the end. If this message is not defined, then the behavior is the same as the backpack plugin.

This addition is good, and the fallback is great, but the number appended in the bracket here is the count of blocks in the backpack, not the multi-selected ones, which is also consistent with the default behavior of the backpack plugin: https://github.com/google/blockly-samples/blob/c32266b2b7685c0d4d6646e5bcedb085c435e0b2/plugins/workspace-backpack/src/backpack_helpers.ts#L115

Previously I have the multi-select blocks count at the start of the string.

[ewpatton]

Unfortunately, that convention came from App Inventor and I have never personally been a fan of it. Having the number of blocks in the backpack listed in the menu option is just noise as it has nothing to do with the blocks to which the menu is actually attached. The backpack plugin took this bad design decision from App Inventor. If we have our choice between showing two numbers in this plugin the appropriate number to show would be the number of workable blocks since that's how much the backpack will grow by (which is what is implemented here).

[mark-friedman]

Fwiw, I agree with Evan..

[HollowMan6]

Okay, actually I don't have preference over these design choices. The core issue I'm focusing on here is consistency. We have to be consistent with the Blockly's backpack plugin, otherwise it will be confusing for users if for some use cases.

Let's assume developers have to init and dispose multi-select plugin back and forth and users may not notice that at all, then we can have:

 return `${Blockly.Msg['COPY_TO_BACKPACK']} (${workableBlocksLength})`;
 return `${Blockly.Msg['COPY_TO_BACKPACK']} (${backpackCount})`;

it's hard for users to determine what the numbers here actually means.

So I think, if you all suggest that we should remove the backpackCount, for sake of consistency, you may want to open a PR there at blockly-samples repo, remove the (${backpackCount}) there, then we can get back to merge this PR. (but lint issues here still needs to be fixed, some lines are too long, check the error with npm run lint).

[mark-friedman]

What if we changed the COPY_TO_BACKPACK message to something like "Copy %1 blocks to backpack". That should make it clear.

[HollowMan6]

What if we changed the COPY_TO_BACKPACK message to something like "Copy %1 blocks to backpack". That should make it clear.

You mean without using i18n (hardcode) the language or something that is already done by Evan?

I've defined COPY_X_TO_BACKPACK in our sources as "Add %1 Blocks to Backpack", which I think is more readable than just appending a number to the end. If this message is not defined, then the behavior is the same as the backpack plugin.

[ewpatton]

I agree that we should propose a similar change to the backpack plugin. In the meantime I have addressed the linter errors.

[mark-friedman]

Oh, sorry. I didn't realize that Evan had already done that. I'm that case I'm even more supportive of his change!

On Thu, Feb 29, 2024, 9:10 AM Hollow Man @.***> wrote:

What if we changed the COPY_TO_BACKPACK message to something like "Copy %1 blocks to backpack". That should make it clear.

You mean without using i18n (hardcode) the language or something that is already done by Evan?

I've defined COPY_X_TO_BACKPACK in our sources as "Add %1 Blocks to Backpack", which I think is more readable than just appending a number to the end. If this message is not defined, then the behavior is the same as the backpack plugin.

— Reply to this email directly, view it on GitHub https://github.com/mit-cml/workspace-multiselect/pull/43#issuecomment-1971589329, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANJWSV4CDB5EMFDZEVDMLTYV5QIJAVCNFSM6AAAAABEAEV62WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRGU4DSMZSHE . You are receiving this because you commented.Message ID: @.***>

[HollowMan6]

I agree that we should propose a similar change to the backpack plugin.

Have we already opened a PR at the blockly-samples repo about this? We can merge this one once we can use the patched backpack plugin version at our side.