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/thebe v2 - interactive figures in articles #136

Merged
merged 56 commits into from
Jul 3, 2023
Merged

Conversation

stevejpurves
Copy link
Member

@stevejpurves stevejpurves commented Jun 23, 2023

Current thebe integration only applies to notebooks, whereas we can include notebook outputs in any markdown file as a figure or embedding.

This PR will replace the thebe provider system with one that can support interactive figures anywhere in a myst site, in addition to still enabling the notebooks to be run and made interactive as before.

Goals:

  • any notebook on a site is identified as "computable" and can be run
  • any article containing a notebook output is identified as "computable" and can be run
  • execution state/context is separate between pages on a site
  • a user can navigate around the site, without losing their session when returning to a page previously visited and run
  • simplified UI for notebooks
  • new compact UI for figures in markdown
  • UI feedback during server connections
  • connections/startups that are initiated will continue to progress even if the user navigates away
  • [DEFERRED] Errors that occur in parts of a notebook that are not attached to the UI are surfaced - see Surface errors in notebook cells not visible on the page #139

@stevejpurves stevejpurves marked this pull request as ready for review July 3, 2023 15:00
@stevejpurves stevejpurves requested a review from rowanc1 July 3, 2023 15:00
@rowanc1
Copy link
Member

rowanc1 commented Jul 3, 2023

I think this is good to go in after a few minor changes that I am making locally. Notes are for future PRs.

There is no error feedback when a server: true fails to connect.

Navigating back to a notebook does not reattach cells (I thought that was in scope?).

Navigating back to an article does not reattach the model correctly.
image

It would be nice to figure out a way to only install things on lite:
image

In lite, there are calls to /pypi/all.json, should this be the place that we put our dependencies?

packages/site/tsconfig.json Outdated Show resolved Hide resolved
@rowanc1 rowanc1 merged commit b81068e into main Jul 3, 2023
1 check passed
@rowanc1 rowanc1 deleted the feat/thebe-v2 branch July 3, 2023 18:41
@stevejpurves
Copy link
Member Author

navigating back should reattach cells -- will investigate.

afaik the /pypi/all.json is more like a fake pypi.org main listing, rather than the list of wheels that should be loaded

<div className="sticky top-[60px] flex justify-end w-full z-20">
<div className="flex p-1 m-1 space-x-1 border rounded-full shadow border-stone-300 bg-white/80 dark:bg-stone-900/80 backdrop-blur">
<div className="sticky top-[60px] flex justify-end w-full z-20 pointer-events-none">
<div className="flex p-1 m-1 space-x-1 border rounded-full shadow pointer-events-auto border-stone-300 bg-white/80 dark:bg-stone-900/80 backdrop-blur">
Copy link
Member Author

Choose a reason for hiding this comment

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

@rowanc1 guessing this was to stop the pointer showing up over blank space in the rest of the div yeah?

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

@@ -1,6 +1,6 @@
{
"extends": "../tsconfig/react-library.json",
"include": [".", "../jupyter/src/execute", "../jupyter/src/controls"],
"include": ["."],
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm wonder if this was a vscode auto thing, as i didnt; add these explicitly

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, when you drag from one folder to another it happens!

Copy link
Member

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

Woops, didn't post my comments!

Comment on lines -20 to +21
"test": "vitest run"
"test": "vitest run",
"test:watch": "vitest watch"
Copy link
Member

Choose a reason for hiding this comment

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

@stevejpurves this is more consistent with mystmd.

@@ -44,6 +45,7 @@ export function ConnectionStatusTray() {

const host = thebe?.useBinder ? 'Binder' : thebe?.useJupyterLite ? 'JupyterLite' : 'Local Server';

// TODO radix ui toast!
Copy link
Member

Choose a reason for hiding this comment

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

Looking forward to this. :)

);
}

export function ErrorTray({ errors }: { errors: IThebeNotebookError[] }) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should also be radix in the future.

Comment on lines +107 to +115
| 'NAVIGATE'
| 'REQUEST_BUILD'
| 'BUILD_STATUS'
| 'CLEAR_BUILD'
| 'ADD_MDAST'
| 'ADD_NOTEBOOK'
| 'ADD_SESSION'
| 'SET_FIRST_EXECUTION'
| 'SET_RENDERING_READY';
Copy link
Member

Choose a reason for hiding this comment

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

We might want to turn this into a type in future (or enum). Might catch a few errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah the typing is not great, so strengthening that is good. I'm getting turned off enums though as they're not types and trying to use the thebe event enums for example are painful.

kind: BusyKind;
}

// TODO action typing is not giving build time type errors :(
Copy link
Member

Choose a reason for hiding this comment

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

An enum would. But that might be overkill.

@@ -1,6 +1,6 @@
{
"extends": "../tsconfig/react-library.json",
"include": [".", "../jupyter/src/execute", "../jupyter/src/controls"],
"include": ["."],
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, when you drag from one folder to another it happens!

<div className="sticky top-[60px] flex justify-end w-full z-20">
<div className="flex p-1 m-1 space-x-1 border rounded-full shadow border-stone-300 bg-white/80 dark:bg-stone-900/80 backdrop-blur">
<div className="sticky top-[60px] flex justify-end w-full z-20 pointer-events-none">
<div className="flex p-1 m-1 space-x-1 border rounded-full shadow pointer-events-auto border-stone-300 bg-white/80 dark:bg-stone-900/80 backdrop-blur">
Copy link
Member

Choose a reason for hiding this comment

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

Yep!

@@ -163,7 +163,7 @@ export const TableOfContents = ({
<div
ref={tocRef}
className={classNames(
'fixed xl:article-grid article-grid-gap xl:w-screen xl:pointer-events-none overflow-auto max-xl:min-w-[300px] z-20',
'fixed xl:article-grid article-grid-gap xl:w-screen xl:pointer-events-none overflow-auto max-xl:min-w-[300px]',
Copy link
Member Author

Choose a reason for hiding this comment

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

@rowanc1 reverting this causes two issues: the menu items can be covered by the toolbar div and the inset black background no longer blacks out the compute toolbar. The toolbar is set to z-10 to lift above content in the main body.

image

did the z-20 here interfere with something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

or is there a better way to deal with this overall?

Copy link
Member

Choose a reason for hiding this comment

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

It breaks the cross-references which appear under the toc.

The pointer-events-none should allow the controls to be mostly fixed though, only the right hand side is active?

Copy link
Member

Choose a reason for hiding this comment

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

Left some other notes on this here:
#136 (comment)

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

2 participants