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

Basic integration of Python Lab with CDO IDE #57935

Merged
merged 9 commits into from Apr 12, 2024

Conversation

molly-moen
Copy link
Contributor

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Summary

V0 of integrating Python Lab with CDO IDE. Python Lab doesn't need the preview panel so it just uses the left and center panels. For now I put the console below both panels, but ideally it should just be below the editor, and be a part of CDO IDE. To keep this scoped that can be a follow-up (the console also is not well styled as of now).

Current UI
Screenshot 2024-04-10 at 1 33 14 PM

Any existing projects won't load correctly because I didn't set an active or open file in the previous configuration. It's probably only me that has any projects anyway, and the page still loads, you just have to delete the old files (via the UI) and create new ones.

As part of this I also refactored Editor.tsx which was previously web lab 2 specific and moved it into the CDO IDE folder. It only had two references to html and css that could be easily turned into parameters.

Links

Testing story

Tested locally. CDO IDE currently has a bug where when you switch levels the project for the previous level overwrites the next level. That is being fixed in #57928.

Follow-up work

Lots of styling updates and improvements still are in progress here and tracked via the Web Lab 2 workstream. From the python lab side, there are a few specific follow ups:

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 review from thomasoniii and a team April 10, 2024 20:41
};

const defaultConfig: ConfigType = {
//showSideBar: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you leave these commented out as an indicator we'll want to change them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly I copied this over from WebLab2View and kept the commented out pieces to indicate we haven't set them yet.

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'll plan to clean these up as a follow-up

Copy link
Contributor

@ebeastlake ebeastlake left a comment

Choose a reason for hiding this comment

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

Nice! 🎉

Comment on lines +77 to +101
const [currentProject, setCurrentProject] = useState<MultiFileSource>();
const [config, setConfig] = useState<ConfigType>(defaultConfig);
const initialSources = useAppSelector(state => state.lab.initialSources);
const channelId = useAppSelector(state => state.lab.channel?.id);
const dispatch = useAppDispatch();

// TODO: This is repeated in Weblab2View. Can we extract this out somewhere?
const setProject = (newProject: MultiFileSource) => {
setCurrentProject(newProject);
dispatch(setSource(newProject));
if (Lab2Registry.getInstance().getProjectManager()) {
const projectSources = {
source: newProject,
};
Lab2Registry.getInstance().getProjectManager()?.save(projectSources);
}
};

useEffect(() => {
// We reset the project when the channelId changes, as this means we are on a new level.
setCurrentProject(
(initialSources?.source as MultiFileSource) || defaultProject
);
}, [channelId, initialSources]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR (unless you want to), but I think yoinking it out into a hook that we can share with weblab sounds good.

Something like this:

// or whatever we call the hook
export const useIDEProject = () => {
  const [currentProject, setCurrentProject] = useState<MultiFileSource>();
  const initialSources = useAppSelector(state => state.lab.initialSources);
  const channelId = useAppSelector(state => state.lab.channel?.id);
  const dispatch = useAppDispatch();

  // I think I memoized this correctly...
  const setProject = useMemo(() => (newProject: MultiFileSource) => {
    setCurrentProject(newProject);
    dispatch(setSource(newProject));
    if (Lab2Registry.getInstance().getProjectManager()) {
      const projectSources = {
        source: newProject,
      };
      Lab2Registry.getInstance().getProjectManager()?.save(projectSources);
    }
  }, [setCurrentProject, dispatch])

  useEffect(() => {
    // We reset the project when the channelId changes, as this means we are on a new level.
    setCurrentProject(
      (initialSources?.source as MultiFileSource) || defaultProject
    );
  }, [channelId, initialSources]);

  return [currentProject, setProject]
}

I think that'll do it, but double check my work. Then in both components we can just do:

const [project, setProject] = useIDEProject()

and then it should handle all of the IO and updates for us and keep it nicely hidden.

It might be useful to have another wrapper component that automatically sets these project/setProject values on the IDE, but that's kinda feeling like a premature optimization atm. If there's a way to add it w/o another wrapper, let's go for it, but otherwise maybe wait a little.

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 made a follow up task to not lose this! https://codedotorg.atlassian.net/browse/CT-499

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.

non-blocking suggestion about abstracting out the todo code into a hook, but otherwise lgtm 🚀

@molly-moen molly-moen merged commit 5b17981 into staging Apr 12, 2024
2 checks passed
@molly-moen molly-moen deleted the molly/python-lab-cdo-ide branch April 12, 2024 16:01
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

3 participants