-
Notifications
You must be signed in to change notification settings - Fork 587
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
Teacher Tool: Refactor Rubric to Checklist #9996
Conversation
…" and "rubric" to "checklist"
…ks/teachertool/rubric_to_checklist
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.
Just a couple of small questions
<p> | ||
{lf("This tool is designed to help you evaluate student projects using a rubric.")}{" "} | ||
{lf("This tool is designed to help you evaluate student projects using an automated checklist.")}{" "} |
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 does "automated checklist" mean versus just "checklist"?
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 just felt "checklist" by itself sounds like it requires the teacher to manually check boxes, wanted to clarify that we'll automatically provide a result.
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.
Hmm, okay. I'm fine with keeping it as is. I think when I read that, I'm thinking that the checklist will get generated for me, but I'm not sure that will be true for everyone.
@@ -30,7 +30,7 @@ function delValue(key: string) { | |||
|
|||
const teacherToolDbName = "makecode-project-insights"; | |||
const dbVersion = 1; | |||
const rubricsStoreName = "rubrics"; | |||
const checklistsStoreName = "checklists"; |
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.
Is there anything extra that we have to do to the rubrics store? I know at this point it's only been us using this, but I was just curious if there was any work needed there.
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.
If we'd shipped it already, yes. Since we haven't, I think it's fine to make the breaking change.
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 did end up bumping the indexedDB version too, so our testers don't have to go and manually clear it for things to work again.
teachertool/src/state/actions.ts
Outdated
rubric, | ||
const setChecklist = (checklist: Checklist): SetChecklist => ({ | ||
type: "SET_CHECKLIST", | ||
checklist: checklist, |
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.
Can we not use the same pattern of just having checklist
as the "checklist" entry here?
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.
Just an automated refactoring artifact, I'll change it.
…ks/teachertool/rubric_to_checklist
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.
LGTM!
These docs files need updating alongside microsoft/pxt#9996
This changes all "Rubric" references to "Checklist". Originally I was just going to change the public-facing strings, but I thought if I did that, we'd likely end up forever chasing down places where we accidentally put Rubric instead of Checklist in a public-facing place, and taking the time to refactor everything reduces the likelihood of that happening...and better to do it now than to wait until the codebase is even bigger.
I also changed "MakeCode Project Insights" to "MakeCode Code Evaluation"
I only found one place (the toolbar action menu dropdown) where styles had to be updated to account for the longer string size, but feel free to play with the upload target and see if you can spot anymore.
Upload Target: https://makecode.microbit.org/app/3cf062d62f2f129c59c7e987ed9249fe9057f38a-cfa942bba5--eval
Fixes microsoft/pxt-microbit#5610