Teacher Tool: Change block display #10202
Conversation
…ks/teachertool/change_block_display
…blocks are defined a bit differently in blocks.ts and/or blockSnippets.ts? Idk this doesn't feel like a good solution...
…ks/teachertool/change_block_display
…ks/teachertool/change_block_display
…ruggled to figure out what to call this one, opted for the simplest approach which is just to include a subset of the options in it. Don't love this, but doing it for now until I can think of something better.
…t. This way we don't have to wait for the toolbox info to display cached results.
| .block-name-param { | ||
| border-radius: 1rem; | ||
| padding: 0.2rem 0.4rem; | ||
| background-color: rgba(0, 0, 0, 0.3); |
There was a problem hiding this comment.
Should we make or use one of the color variables here? I don't have a strong preference, but am curious if there might be a use case elsewhere for a slightly translucent black.
| // Check for cached version. | ||
| let readableBlockName = getCachedBlockAsText(blockId); | ||
| if (readableBlockName) { | ||
| return Promise.resolve(readableBlockName); |
There was a problem hiding this comment.
Is this Promise.resolve needed here? If a truthy value is returned from this function, doesn't that mean the promise has resolved?
There was a problem hiding this comment.
async functions don't need Promise.resolve like this, yeah
| params.unshift(null); | ||
| } | ||
| parts.forEach((part, i) => { | ||
| const blockAsText = getBlockAsText(block, params, isPython); |
There was a problem hiding this comment.
Ooh, this is a very nice improvement!
|
Unrelated to code or anything, I just wanted to say your PR description is one of my favorites that I've seen. In addition to being very readable, the subcategory titles are awesome. I think I'm going to start using "Free Trial" with my upload targets now 😄. |
| // Check for cached version. | ||
| let readableBlockName = getCachedBlockAsText(blockId); | ||
| if (readableBlockName) { | ||
| return Promise.resolve(readableBlockName); |
There was a problem hiding this comment.
async functions don't need Promise.resolve like this, yeah
This is the Writer in Thomas coming out. I like the parts where weird decisions are being explained ;-) |
This changes the inline block display for the "Block used count times" criteria in the teacher tool. I've reused the approach taken by the monaco editor to generate the javascript toolbox entries (meaning some of that code has been refactored into a shared helper method). This approach looks at the attributes for a block (mostly the block string), then breaks it up into labels, parameters, and breaks. Those sections are returned and can be displayed however the caller sees fit. I cache the information to minimize any loading time (fewer messages to the iframe) and to reduce the likelihood of ending up with a block where we can't get a display (i.e. a rubric with a block loaded from an extension, opened later from a project without that extension installed).
This changes the inline block display for the "Block used count times" criteria in the teacher tool. I've reused the approach taken by the monaco editor to generate the javascript toolbox entries (meaning some of that code has been refactored into a shared helper method). This approach looks at the attributes for a block (mostly the block string), then breaks it up into labels, parameters, and breaks. Those sections are returned and can be displayed however the caller sees fit. I cache the information to minimize any loading time (fewer messages to the iframe) and to reduce the likelihood of ending up with a block where we can't get a display (i.e. a rubric with a block loaded from an extension, opened later from a project without that extension installed).
This changes the inline block display for the "Block used count times" criteria in the teacher tool. I've reused the approach taken by the monaco editor to generate the javascript toolbox entries (meaning some of that code has been refactored into a shared helper method). This approach looks at the attributes for a block (mostly the block string), then breaks it up into labels, parameters, and breaks. Those sections are returned and can be displayed however the caller sees fit. I cache the information to minimize any loading time (fewer messages to the iframe) and to reduce the likelihood of ending up with a block where we can't get a display (i.e. a rubric with a block loaded from an extension, opened later from a project without that extension installed).
Overview
This changes the inline block display for the "Block used count times" criteria in the teacher tool. I've reused the approach taken by the monaco editor to generate the javascript toolbox entries (meaning some of that code has been refactored into a shared helper method). This approach looks at the attributes for a block (mostly the block string), then breaks it up into labels, parameters, and breaks. Those sections are returned and can be displayed however the caller sees fit.
I cache the information to minimize any loading time (fewer messages to the iframe) and to reduce the likelihood of ending up with a block where we can't get a display (i.e. a rubric with a block loaded from an extension, opened later from a project without that extension installed).
The part where I explain some weird decisions
There are a few things I don't love about this change, mostly revolving around built-in blocks which lack the attributes used to generate the different "parts". In these cases, the monaco editor uses snippetNames defined on the blocks in monacoSnippets.ts. I've opted for something similar, but since the monaco toolbox lacks some of the blocks that the block-based toolbox has (on start, repeat, etc...), I've gone ahead and updated the blockSnippets.ts file instead of trying to reuse the monaco ones. There's some duplication here, but I think this feels like the right place for it regardless.
The part where I explain some weirder decisions
Some blocks (specifically, some built-in blocks) have multiple entries in the toolbox which result in blocks of the same id. For example, we have different toolbox entries for plus, minus, times, and divide, but they're all the same math_arithmatic block, so we only show one entry in the block picker and it passes on any of those blocks (since BlockExists validation is based on blockId). In an ideal world, I'd change the BlockExist validator to allow us to check for these separately, but that's out of scope for the current teacher tool work. Instead, I've created a workaround where multiple matches are simply combined into one string, concatenated with " | ":
Similarly, there are some blocks that only appear in the toolbox once but still have multiple different dropdown options that make it tricky to find a descriptive overall snippet name. (Notably, the "Square Root" and "Round" blocks.) In these, I've gone with a similar approach of just piping together the different operations in the snippet name. I look forward to the day I can change BlockExist and remove all this code 🙂
Free Trial
Upload Target: https://makecode.microbit.org/app/c07bd3857b9afa5e604d68f1d16dcbe2bd536dcd-bb5f0ab19d
Screenshots
Before
After
