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

[Google Blockly] Support block limits #57957

Merged
merged 12 commits into from Apr 12, 2024
Merged

[Google Blockly] Support block limits #57957

merged 12 commits into from Apr 12, 2024

Conversation

mikeharv
Copy link
Contributor

@mikeharv mikeharv commented Apr 11, 2024

About Block Limits

In 2016, we built a custom CDO Blockly feature for Maze, Artist, and Play Lab called block limits. This feature was designed to encourage students to think about their programming solutions in more efficient ways. Originally, this feature prevented students from using more than a set limit of a particular type of block. Google Blockly supports this with a maxInstances option (documentation, example). However, our feature was later relaxed so that students could still drag out as many blocks as they wanted, but their pass/fail feedback would still reflect the limits. This was intended to allow students to start with an inefficient solution and then refactor (e.g. using a loop).

image

How it works:

  • To set a block limit of 1, a limit="1" attribute is added to the block element of the toolbox XML.
  • In the student view of the level, an SVG element is rendered at the top left of the block initially showing the limit.
  • As students drag out blocks (and enable them by connecting to stack), the counter over the block counts down.
  • If the student has used more than the limit, the SVG counter shows ! and changes color.
  • If the student otherwise passes the level but with block than the limit of any available block, they get fail the level with a generated feedback string. The feedback includes a restatement of the limit and a rendering of the block.

This branch makes all of the above work with Google Blockly:
image

Technical Details

Storing Limits
The first challenge was to find a way to be able to read and store the block limits. Blockly doesn't expect extra state to be serialized as attributes in the block element, instead preferring the use of mutators. Since we were already storing the toolbox xml string on the wrapper, I created a util createBlockLimitMap() that parses the xml to find these values. They are added to a map (<string, number>. This map is placed directly on the wrapper so that it can accessed as Blockly.blockLimitMap in other files.

Event Handler
Because the SVG will need to be updated, I registered a workspace change listener for blocks. This handler creates or updates a blockCountMap that also put on the wrapper instance. This is populated with the current workspace count of blocks whose types are represented in the limit map. (If a block type doesn't have a limit, we don't bother to count the instances of that block.)
Once we have the counts, we then create (or update) an SVG for each flyout block with a corresponding type. The SVG is given the number of blocks "remaining" (subtracting the count from the limit).

Limit SVG
In CDO Blockly, the limit is managed directly by the BlockSvg class. Since this mainline doesn't support extending this class for purposes like this, I created a new BlockSvgLimit class that does pretty much the same thing. When needed, we will create an instance of this class and append it to the block. The actual SVG drawing is the same as the original with the exception of two minor changes:

  • The colors have been updated in accordance with guidance from the Design team (@markabarrett). The new colors are listed in cdoCss.
  • Bubble height and text position have been tweaked for better centering and to account for changing themes.

Original code for reference: https://github.com/code-dot-org/blockly/blob/v4.1.0/core/ui/block_svg/block_svg.js#L879-L951

Validation

Again, this feature doesn't prevent students from using too many blocks, but it does prevent that from passing if they have. The functions blockLimitExceeded and getBlockLimit are used to make this determination. These functions were already defined in cdoUtils but not fully implemented. They now work by reading from the needed map(s) to determine if a block type and limit needs to be added to the failure feedback string.
The CDO Blockly versions of these functions are here: https://github.com/code-dot-org/code-dot-org/blob/mike/block-limits/apps/src/blockly/cdoBlocklyWrapper.js#L253-L267
The BlockLimits functions referenced at the link above are part of the CDO Blockly wrapper and available here: https://github.com/code-dot-org/blockly/blob/v4.1.0/core/ui/block_space/block_limits.js

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Links

Jira: https://codedotorg.atlassian.net/browse/CT-495
Slack #Design thread: https://codedotorg.slack.com/archives/C0SUN2SSF/p1712781058747139

Testing story

I manually tested that the new SVG works as expected and is resized appropriately for a variety of numeric values. Students can pass or a fail a level as expected based whether they've exceeded a block limit. The feedback string renders the correct block and limit. I also made sure that the bubbles update in real time if the theme is changed.

We already have integration tests for block limits that use maze, so once Maze is migrated our coverage of this feature will pivot to cover the new implementation for Google Blockly.

Deployment strategy

Follow-up work

This work does not cover levelbuilder's ability to set block limits and save them when editing toolbox blocks.. If that feature is still desired we would need to:

  • show the new svg on workspace blocks if we are in toolbox edit mode
  • register a new block context menu option to update values in the block limit map
  • save values from the map into the xml when saving toolbox blocks

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@mikeharv mikeharv marked this pull request as ready for review April 11, 2024 16:04
@mikeharv mikeharv requested a review from a team April 11, 2024 16:04
export function updateBlockLimits(event: Abstract) {
// This check is to update bubbles that show block limits whenever
// blocks on the main workspace are updated.
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We could add an isRelevantEvent helper for this.

if (!isRelevantEvent(event)) {
  return;
}

Copy link
Contributor Author

@mikeharv mikeharv Apr 11, 2024

Choose a reason for hiding this comment

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

Would that help with readability? Could also do something like
![...expectedEventTypes].includes(event.type)

Copy link
Contributor

@molly-moen molly-moen 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, a couple comments/questions!

apps/src/blockly/addons/blockSvgLimitIndicator.ts Outdated Show resolved Hide resolved
apps/src/blockly/addons/blockSvgLimitIndicator.ts Outdated Show resolved Hide resolved
apps/src/blockly/addons/cdoUtils.ts Outdated Show resolved Hide resolved
apps/src/blockly/eventHandlers.ts Outdated Show resolved Hide resolved
const blockCountMap = new Map<string, number>();
Blockly.blockCountMap = blockCountMap;
// Initialize block counts based on blockLimitMap
blockLimitMap.forEach((_, type) => {
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 we need to initialize all of these to zero?

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 think we want to make sure that both maps include the same keys (block types). Initializing to 0 ensures that blocks that the student hasn't used yet are still list in the count map.

@mikeharv mikeharv merged commit 33fa262 into staging Apr 12, 2024
2 checks passed
@mikeharv mikeharv deleted the mike/block-limits branch April 12, 2024 19:57
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