-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
75f7f92
to
376b86d
Compare
navigating back should reattach cells -- will investigate. afaik the |
<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"> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
packages/site/tsconfig.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"extends": "../tsconfig/react-library.json", | |||
"include": [".", "../jupyter/src/execute", "../jupyter/src/controls"], | |||
"include": ["."], |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this 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!
"test": "vitest run" | ||
"test": "vitest run", | ||
"test:watch": "vitest watch" |
There was a problem hiding this comment.
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! |
There was a problem hiding this comment.
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[] }) { |
There was a problem hiding this comment.
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.
| 'NAVIGATE' | ||
| 'REQUEST_BUILD' | ||
| 'BUILD_STATUS' | ||
| 'CLEAR_BUILD' | ||
| 'ADD_MDAST' | ||
| 'ADD_NOTEBOOK' | ||
| 'ADD_SESSION' | ||
| 'SET_FIRST_EXECUTION' | ||
| 'SET_RENDERING_READY'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :( |
There was a problem hiding this comment.
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.
packages/site/tsconfig.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"extends": "../tsconfig/react-library.json", | |||
"include": [".", "../jupyter/src/execute", "../jupyter/src/controls"], | |||
"include": ["."], |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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]', |
There was a problem hiding this comment.
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.
did the z-20
here interfere with something else?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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: