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

Add "Start Over" to CodeBridge #58534

Merged
merged 7 commits into from May 13, 2024
Merged

Conversation

molly-moen
Copy link
Contributor

This adds a start over button to the CodeBridge workspace. To get this working I had to do a bit of refactoring:

  • Instead of configuring the file tabs and editor as separate components in the layout, they are combined as oneworkspace component. In order to enable this I removed the direct passing of the editor component and instead pass the language mapping and editable file types to the config. This also had the benefit of moving the header for the editor above the file tabs.
  • I added the same start over icon button to the new "Workspace" header that we use in Music Lab.
  • I updated the useSources hook to return a start over function as well. I was able to reuse logic from Music Lab/lab2 for this.
  • I re-used the same start over dialog from Music Lab/lab2. This dialog specifically mentioned "blocks" in the text. I updated it to check the lab type, and show "code" instead if the lab is text-based.

The new flow now looks like this (it is very similar in Web Lab 2):
https://github.com/code-dot-org/code-dot-org/assets/33666587/1ea7ec39-0321-43ae-a6a4-ff7e7b92c51f

Links

Testing story

Tested locally in Python Lab and Web Lab 2

Follow-up work

We have a separate task to add full version history.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@molly-moen molly-moen requested a review from a team as a code owner May 10, 2024 20:03
@@ -11,13 +13,21 @@ const StartOverDialog: React.FunctionComponent<BaseDialogProps> = ({
handleConfirm,
handleCancel,
}) => {
const currentAppName = useAppSelector(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sanchitmalhotra126 I'm pretty confident this won't impact Music Lab since we are only using the new translated string if we are in a text-based lab. LMK if this is too risky during the launch period.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to me 👍 just pulled this locally and confirmed on my end as well.

@@ -208,8 +204,6 @@ const Weblab2View = () => {
if (configName === 'project') {
setSource(newConfig as ProjectType);
} else if (configName === 'config' || configName === 'layout') {
(newConfig as ConfigType).EditorComponent =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomasoniii is it ok that I removed this since we don't support providing an editor component anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. That's a shame. I thought the flexibility would've been nice.

Anyway, sure, if not supported can be safely tossed.

Copy link
Contributor

@mikeharv mikeharv left a comment

Choose a reason for hiding this comment

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

Looks good! I think it's worth cleaning up the grid rows before merging, now that we don't need to worry about file tabs separately.

apps/i18n/common/en_us.json Show resolved Hide resolved
apps/src/codebridge/Editor/index.tsx Show resolved Hide resolved
'javalab',
'weblab',
'pythonlab',
'weblab2',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an exhaustive list? Are applab and weblab compatible with Lab2? If "not yet", maybe we should also include gamelab? Not sure if there are other worth considering too, like ai lab.

Alternatively, I wonder if it might be more robust to actually check whether the level uses Blockly or not.

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 this was my loose list :) I put labs that I knew were text-based, and included those not on lab2 so it wasn't something we had to worry about in the future. But you are probably right that having a half-correct list isn't great. I can update this for now to just be lab2 labs that are text based, and when a lab is migrated when can add it to the list. I'm not sure if we can check consistently if a level uses Blockly, is that a setting on every lab type?

Copy link
Contributor

Choose a reason for hiding this comment

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

All Blockly levels, including Music, should extend the Blockly level class. I've seen us do level.is_a? Blockly in some places. I'm not sure if that's helpful or not.
I'd vote for either being robust like this (if it works) or having a short list that just has supported lab2 labs if that doesn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do that on the frontend without sending that as a level property (which I don't think we do now). I'll pare down the list and comment that it's only lab2 labs.

"instructions editor"
"file-browser editor"
"instructions workspace"
"instructions workspace"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can clean this up now, by removing one of these duplicate lines. Above on line 70 we can just specify two rows. Instead of giving the rows a fixed height, we could try make it flexible like

gridLayoutRows: '1fr 1fr',

which would give each 50% of the available space.

We could do similar improvements to the weblab2 layouts since they also have a couple of identical rows now.

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 might leave this for now since I'm going to work next on moving the console into Code Bridge, and this will get updated again.

Copy link
Contributor

Choose a reason for hiding this comment

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

The duplicate line can safely be deleted, and the layoutRows can be simplified as well.

But also, not urgent especially if more layout changes are pending.

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'm going to fast follow this with an update to both layouts!

Comment on lines +22 to +31
<div>
<Button
icon={{iconStyle: 'solid', iconName: 'refresh'}}
isIconOnly
color={'black'}
onClick={onClickStartOver}
ariaLabel={'Start Over'}
size={'xs'}
/>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be wrapped in the extra div? Is it just anticipating more buttons being added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah just anticipating more buttons.

Comment on lines 23 to +34
gridLayoutColumns: '300px auto auto',
gridLayout: ` "instructions file-tabs preview-container"
"instructions editor preview-container"
"file-browser editor preview-container"`,
gridLayout: ` "instructions workspace preview-container"
"instructions workspace preview-container"
"file-browser workspace preview-container"`,
};

const verticalLayout = {
gridLayoutRows: '32px 300px auto auto',
gridLayoutColumns: '300px auto',
gridLayout: ` "instructions file-tabs file-tabs"
"instructions editor editor"
"file-browser editor editor"
gridLayout: ` "instructions workspace workspace"
"instructions workspace workspace"
"file-browser workspace workspace"
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 the PythonLab comments above, we can simplify the gridLayoutRows and redundant gridLayout line.

Copy link
Contributor

@thomasoniii thomasoniii left a comment

Choose a reason for hiding this comment

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

minor parroting on the grid layout notes, but otherwise lgtm 🚀

@molly-moen molly-moen merged commit a03ff71 into staging May 13, 2024
2 checks passed
@molly-moen molly-moen deleted the molly/start-over-codebridge branch May 13, 2024 17:16
@molly-moen molly-moen mentioned this pull request May 13, 2024
8 tasks
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.

None yet

4 participants