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

feat: Create notebook copies #2227

Merged
merged 11 commits into from
Sep 5, 2024
Merged

feat: Create notebook copies #2227

merged 11 commits into from
Sep 5, 2024

Conversation

wasimsandhu
Copy link
Collaborator

@wasimsandhu wasimsandhu commented Sep 5, 2024

📝 Summary

Adds "Create notebook copy" option to the notebook actions menu.
Closes #2068.

Screenshot 2024-09-04 at 11 10 57 PM

🔍 Description of Changes

  • Create /api/kernel/copy endpoint
  • Add "Create notebook copy" option to notebook dropdown menu
  • After user creates notebook copy, open in new tab

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka
@mscolnick

@wasimsandhu wasimsandhu added the enhancement New feature or request label Sep 5, 2024
@wasimsandhu wasimsandhu self-assigned this Sep 5, 2024
Copy link

vercel bot commented Sep 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 5:09pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 5:09pm

Copy link
Contributor

@mscolnick mscolnick left a comment

Choose a reason for hiding this comment

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

Nice looks great! I think just some small tweaks and it's there

<div>
<SessionShutdownButton filePath={file.path} />
</div>
<CopyNotebookButton filePath={file.path} />
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 put this in notebook actions - so it automatically appears in the notebook dropdown and in the command palette. It doesn't need to be its own button.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good! I was just going off the issue author's suggestions, thought it'd be a simple add

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yea I didn't see the second part of the issue mentioning that. I don't know how common it'll be and does complicate the routes. I think we hold off homepage and consider later

<SessionShutdownButton filePath={file.path} />
</div>
<CopyNotebookButton filePath={file.path} />
<SessionShutdownButton filePath={file.path} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this on the homepage - at least for now.

This is also probably why you were getting mismatch sessions. Since the homepage doesn't have an active session

@@ -187,6 +187,11 @@ export class PyodideBridge implements RunRequests, EditRequests {
return null;
};

sendCopy: EditRequests["sendCopy"] = async () => {
// TODO: Implement copy with pyodide!
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can throw not implemented. And I think we can hide this action in the notebook dropdown when isWasm is true

if (!source) {
return null;
}
const pathBuilder = new PathBuilder("/");
Copy link
Contributor

Choose a reason for hiding this comment

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

const pathBuilder = PathBuilder.guessDeliminator(source)

filename,
destination,
);
window.open(notebookCopy);
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just directly do:

window.open(`/?file=${destination}`, "_blank");

window.open(notebookCopy);
})
.catch((error) => {
toast({
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 already toast from the request wrapper in requests-toasting.tsx

@mscolnick mscolnick merged commit 4d6b93f into main Sep 5, 2024
31 checks passed
@mscolnick mscolnick deleted the ws/copy-notebook branch September 5, 2024 17:15
Copy link

github-actions bot commented Sep 5, 2024

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.8.12-dev7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a Copy Notebook Option
2 participants