-
Notifications
You must be signed in to change notification settings - Fork 480
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
[Sprite Lab] Validation updates for CSF pilot #45021
Conversation
@@ -69,3 +69,13 @@ function storeVariableLogforPrevious() { | |||
previousVarLog = JSON.parse(JSON.stringify(varLog)); | |||
} | |||
|
|||
// Returns true if the student has a variable label that starts with "_" | |||
function noBadVariableLabels() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder if we can name this more a bit more specific to what this function is doing. Maybe something like allowBeginningWithUnderscore
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reversed the logic and renamed to varLabelStartsWithUnderscore()
. Updated each level that used the function.
if (label.charAt(0) == "_") { | ||
result = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use ===
here because it will compare value as well as type.
Also, not totally sure how result
is used, but isn't this returning false if the variable starts with _
? The comment says it should return true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catches, fixed!
@@ -49,11 +49,14 @@ export const commands = { | |||
}, | |||
|
|||
// Return true if any sprite's speech include values from a given object. | |||
anySpeechIncludesValues(object) { | |||
anySpeechIncludesValues(object, object2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are object
and object2
? If it's currentVar
and previousVar
we should probably call them that so it's clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this piece in your PR description was really helpful context:
in case the author would like to give the flexibility of printing the value from either the current or previous frame
Could be good to summarize in the comment as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this and wasn't sure what made the most sense. Labels like currentVar
reflect the intended use, but technically the function could be used with any object(s).
How can I test this locally? |
…-dot-org into csf-validation-updates
This reverts commit 5983eab.
@katleiahramos You can test this by clicking through the levels at http://localhost-studio.code.org:3000/s/coursef-2022/ The main ones updated here are:
|
These changes need to reach levelbuilder tonight, Thursday, February 24 if at all possible. This will enable final adjustments to be made by the Curriculum Team tomorrow in advance of the pilot beginning Monday, February 28.
After reviewing the new validation for CSF pilot lessons, I was asked to make three quick improvements:
First, from Emma:
This is now addressed with the `noBadVariableLabels()` function that runs in the interpreter. It actually impacts multiple levels and not just the one that was reported. (Note that "???" is an invalid variable name and converted behind-the-scenes to "___", which *is* valid.
Second, when students were expecting to have a sprite say a value from a variable, they might have failed depending on the order of the blocks. It is now possible to pass two objects into the function, in case the author would like to give the flexibility of printing the value from either the current or previous frame. See update to
criterionCommands.js/anySpeechIncludesValues()
.Expected solution:
Unexpected solution (fails, but should pass):
Both examples now pass validation.
Finally, the color of the validation bars was changed from red/green to purple/teal to align to the other CSF lessons that use older validation.