-
Notifications
You must be signed in to change notification settings - Fork 593
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: Block Picker #9936
Conversation
…ate the block xml
…ow we would render blocks with dropdowns.
…ks/teachertool/block_picker
…ks/teachertool/block_picker
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.
Looking great. Made some minor comments/suggestions.
className={classList(css["category-block-list"], expanded ? css["expanded"] : css["collapsed"])} | ||
> | ||
{category.blocks.map(block => ( | ||
<PickBlockButton block={block} category={category} onBlockSelected={blockSelected} /> |
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.
missing key prop
|
||
// Scan all categories and find the block with the matching id | ||
for (const category of Object.values(teacherTool.toolboxCategories)) { | ||
for (const block of category.blocks ?? []) { |
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.
Small suggestion... you could use find
here:
const block = category.blocks?.find(b => b.blockId === param.value);
if (block) {
return { category, block };
}
padding: 0.5rem; | ||
text-align: start; | ||
font-weight: 800; | ||
font-size: 1.3em; |
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.
font-size: 1.3em; | |
font-size: 1.3rem; |
unless you wanted to inherit size units from parent element?
border-radius: 0.2rem; | ||
color: white; | ||
border: 1px solid rgba(0, 0, 0, 0.5); | ||
font-size: 1em; |
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.
same as above
font-size: 1em; | |
font-size: 1rem; |
} | ||
|
||
function shouldIncludeCategory(category: pxt.editor.ToolboxCategoryDefinition) { | ||
return category && category.name && category.blocks?.length != 0; |
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.
According to the parameter definition, category
cannot be null/undefined here. If it can be, indicate with category?: ...
.
return category && category.name && category.blocks?.length != 0; | |
return category.name && category.blocks?.length != 0; |
} | ||
} | ||
|
||
export async function getToolboxCategories( |
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.
export async function getToolboxCategories( | |
export async function getToolboxCategoriesAsync( |
@@ -28,6 +28,9 @@ export function setEditorRef(ref: HTMLIFrameElement | undefined) { | |||
}); | |||
driver.addEventListener("editorcontentloaded", ev => { | |||
AutorunService.poke(); | |||
|
|||
// Reload all blocks. | |||
loadToolboxCategoriesAsync(); |
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.
await this?
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 it's okay to just let this run in the background without awaiting. I'll update comments to indicate that's intentional.
teachertool/src/state/state.ts
Outdated
@@ -15,6 +15,9 @@ export type AppState = { | |||
validatorPlans: pxt.blocks.ValidatorPlan[] | undefined; | |||
autorun: boolean; | |||
confirmationOptions: ConfirmationModalOptions | undefined; | |||
blockPickerOptions: BlockPickerOptions | undefined; |
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.
At some point we may want to have a single modalOptions
field as a compound type, e.g.: modalOptions: ConfirmationModalOptions | BlockPickerOptions | undefined
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 went ahead and combined them, though I took a slightly different approach. May have gone a little overboard with it, but I wanted to avoid blindly casting to the desired types inside the modals themselves, so I ended up making an interface for the options with a modal field we can check against. LMK if you think it's too much.
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.
Your changes look fine, but I think it could be simplified a bit:
- Remove
modal
fromAppState
. The presence of amodalOptions
is enough to know if a modal is showing, and which one. - Use a union of types for the different modal options, with no base:
export interface ConfirmationModalOptions {
modal: "confirmation";
title: string;
message: string;
onCancel: () => void;
onContinue: () => void;
}
export interface BlockPickerOptions {
modal: "block-picker";
criteriaInstanceId: string;
paramName: string;
}
export type ModalOptions = ConfirmationModalOptions | BlockPickerOptions;
- For showing a modal, implement a single
showModal
transform that takes aModalOptions
parameter:
export function showModal(modalOptions: ModalOptions) {
// dispatch the action
...
}
…tentionally not awaiting
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.
Made one more suggestion for modal options, but looks great.
This change adds a "Block Picker" which can be used for customizable block parameters. Doing this involved:
Upload Target:
https://makecode.microbit.org/app/400c169359d2defd8af3c8aa838f740f3a932406-c40b446680--eval?tc=1https://makecode.microbit.org/app/a60b885ddbfa1cf3cc0f4407e122347ce0c1ebf9-d07aa77cc2--eval?tc=1Screenshots
(In order to prevent modal resizing, the modal is large even when all categories are collapsed)
Expand to see blocks
I change the color of the button but do not insert an image of the block in the rubric. I think the actual blocks would be too big a lot of the time: