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

[CRT-32] Add assertions to scratch #50

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

Tyratox
Copy link
Contributor

@Tyratox Tyratox commented Nov 1, 2024

No description provided.

@Tyratox Tyratox requested a review from pierluca November 1, 2024 17:18
@Tyratox Tyratox self-assigned this Nov 1, 2024
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

This PR seems (relatively) further away from being mergeable, the code has a few rough edges. You might also want to fix the CI 😄

A UI question: do the blocks disappear from the toolbox when we've run out of them, or do we display them as unavailable / 0 blocks left? We might want to clarify the desired behaviour. I can see pros/cons from a pedagogical perspective.

apps/scratch/src/components/TaskConfig.tsx Show resolved Hide resolved
apps/scratch/src/extensions/assertions/music.png Outdated Show resolved Hide resolved
apps/scratch/src/extensions/assertions/test-icon-black.svg Outdated Show resolved Hide resolved
apps/scratch/src/extensions/index.tsx Outdated Show resolved Hide resolved
apps/scratch/src/extensions/index.tsx Outdated Show resolved Hide resolved
apps/scratch/types/scratch-vm.d.ts Outdated Show resolved Hide resolved
apps/scratch/types/scratch-vm.d.ts Outdated Show resolved Hide resolved
Base automatically changed from feature/CRT-30_freeze_task_code_2 to main November 4, 2024 18:38
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

Minor improvements are possible, but LGTM overalls!

backend/src/ast/converters/scratch/helpers.ts Outdated Show resolved Hide resolved
async pressGreenFlag() {
return this.page
.getByTestId("stage-controls")
.locator("img:first-child")
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume no other IDs are available? 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, unfortunately not. we would need to copy over the "Controls" component to add testids. we could do that but I think I would rather go with this for now to avoid having too much scratch code copied over?

apps/scratch/src/pages/Solve.tsx Outdated Show resolved Hide resolved
apps/scratch/src/pages/Solve.tsx Outdated Show resolved Hide resolved
apps/scratch/src/pages/Edit.tsx Outdated Show resolved Hide resolved
AppIFrameMessage,
AppIFrameRequest,
AppIFrameResponse,
} from "../../../../frontend/src/types/app-iframe-message";
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we now have a lot of relative paths with 3 or more levels of ../. It's not super obvious where we're importing from anymore, and since we're clearly leaving the scope of the current path, wouldn't it be clearer at that point to switch to absolute paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, that was automatically done by the IDE but you are right ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we need a alias because this comes from a different project ^^ So maybe just change this once we have our external library for the iframe communication?

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