-
Notifications
You must be signed in to change notification settings - Fork 12
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
🕹 Move controls to embed node #142
Conversation
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.
A few comments @stevejpurves, I think this is complete enough to come in.
@@ -1,6 +1,7 @@ | |||
.hover-card-content { | |||
animation-duration: 0.6s; | |||
animation-timing-function: cubic-bezier(0.16, 1, 0.3, 1); | |||
z-index: 10; |
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.
This is the only real style change for z-index.
{canCompute && article.kind === SourceFileKind.Article && <NotebookToolbar />} | ||
{/* {canCompute && article.kind === SourceFileKind.Article && <NotebookToolbar />} */} |
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 have commented this out, but we may want to bring something like it back.
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 like the idea of making it figure oriented -- all we are losing here is the ability to "run all" in one go
import { select } from 'unist-util-select'; | ||
import { useLinkProvider } from '@myst-theme/providers'; | ||
|
||
function EmbedWithControls({ |
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 have moved the embed -> output
node with controls here. This allows us to link to the notebook source.
I am not totally happy with it, but I think it is better than the controls being fully on the right.
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.
💥
This moves the in-article controls to the top of the embed node and also fixes the z-index issues we talked about in #140. I have also removed the article notebook toolbar, I think we could bring it back, but I didn't really think it was necessary? Linked notebooks still work together!