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

Teacher Tool: Block Picker #9936

Merged
merged 59 commits into from
Mar 27, 2024
Merged

Teacher Tool: Block Picker #9936

merged 59 commits into from
Mar 27, 2024

Conversation

thsparks
Copy link
Contributor

@thsparks thsparks commented Mar 22, 2024

This change adds a "Block Picker" which can be used for customizable block parameters. Doing this involved:

  1. Making it so the iframe always loads something, even when no sharelink has been provided. We need this so we can call into it to get the blocks. It's invisible when there is no share link, but it's still loaded.
  2. Adding commands we can send to the iframe to get all the toolbox categories & blocks, then to get images of blocks (as xml, when we have it, or simply by id)
  3. Adding a new modal for the block picker, which displays categories and blocks to choose from
  4. Some adjustments to the LazyImage to make the loading look a little smoother (I think)

Upload Target: https://makecode.microbit.org/app/400c169359d2defd8af3c8aa838f740f3a932406-c40b446680--eval?tc=1 https://makecode.microbit.org/app/a60b885ddbfa1cf3cc0f4407e122347ce0c1ebf9-d07aa77cc2--eval?tc=1

Screenshots

(In order to prevent modal resizing, the modal is large even when all categories are collapsed)
image

Expand to see blocks
image

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:
image

@thsparks thsparks requested a review from a team March 22, 2024 22:24
Copy link
Contributor

@eanders-ms eanders-ms left a 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} />
Copy link
Contributor

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 ?? []) {
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Suggested change
font-size: 1em;
font-size: 1rem;

}

function shouldIncludeCategory(category: pxt.editor.ToolboxCategoryDefinition) {
return category && category.name && category.blocks?.length != 0;
Copy link
Contributor

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?: ....

Suggested change
return category && category.name && category.blocks?.length != 0;
return category.name && category.blocks?.length != 0;

}
}

export async function getToolboxCategories(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

await this?

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 it's okay to just let this run in the background without awaiting. I'll update comments to indicate that's intentional.

@@ -15,6 +15,9 @@ export type AppState = {
validatorPlans: pxt.blocks.ValidatorPlan[] | undefined;
autorun: boolean;
confirmationOptions: ConfirmationModalOptions | undefined;
blockPickerOptions: BlockPickerOptions | undefined;
Copy link
Contributor

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

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 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.

Copy link
Contributor

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 from AppState. The presence of a modalOptions 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 a ModalOptions parameter:
export function showModal(modalOptions: ModalOptions) {
   // dispatch the action
    ...
}

Copy link
Contributor

@eanders-ms eanders-ms left a 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.

@thsparks thsparks merged commit a344259 into master Mar 27, 2024
6 checks passed
@thsparks thsparks deleted the thsparks/teachertool/block_picker branch March 27, 2024 00:06
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.

2 participants