-
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
🚀 MVP launch buttons for JupyterHub & MyBinder #503
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Quick thoughts from me:
Overall - the UX you describe above seems like a good first step for an MVP - it roughly mimics the behavior of Jupyter Book and would let users get started if they were already relying on that feature. |
@@ -24,6 +24,7 @@ | |||
"@heroicons/react": "^2.0.18", | |||
"@radix-ui/react-icons": "^1.3.2", | |||
"@radix-ui/react-popover": "^1.1.2", | |||
"@radix-ui/react-tabs": "^1.1.1", |
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.
👍🏼
function LaunchButton(props: { github: string; location: string; binder?: string }) { | ||
const binder = props.binder ?? 'https://mybinder.org'; | ||
type CommonLaunchProps = { | ||
github: string; |
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.
should we generalize in our naming a bit more? as in:
git: <url>
provider: 'github'
thebe does that, as the intention is to support git interface/protocol.
or are we happy with
github: 'https://gitlab.myinstitution.org/...'
Also I realise that we only have github
in the frontmatter, but I am imagining that this git info will in future come from a more dedicated frontmatter.services
block or the like which may have non github specific fields, like the one I suggest there.
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.
Yes, entirely! I'm going to take inspiration from some of the jb1 config here.
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 we do need to change our frontmatter; GitHub should not be the only Git repo that we can define in our frontmatter.
I would probably have frontmatter fields for github
, gitlab
, etc. that are normalised to a git
field i.e. mystmd
should know how to build canonical URLs for those providers.
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.
💯
d35cb27
to
e2f97b8
Compare
I've added support for basic validation using Radix Form: I think this is good to go for a WIP. I'm keen to ship this in time for AGU, so I'm not exporting it intentionally. We can also disable it by default if that provides less concern for @stevejpurves et al. to ship. Any review welcome. |
@jmunroe this PR adds an analogue to the "launch" button on JB1 (rocket). As an MVP, is this useful for AGU? If so, can you provide a glance over this thread to see if anything stands out as missing? |
export type LaunchOptions = { | ||
repo: string; | ||
location: string; | ||
binder?: string; | ||
jupyterhub?: string; | ||
}; | ||
|
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 don't love this; I think instead frontmatter
should be resolved according to https://mystmd.org/guide/frontmatter#table-frontmatter. But, for now, let's just get this in.
OK I finally got this working locally. Here are a few pieces of feedback Overall: I think it's safe to ship as long as folks don't see red flags with the implementationThis feels like a useful first iteration for this functionality and I'd be fine shipping it to start getting user feedback. I think there is some big functionality missing (in particular, pre-configuring the parameters in myst.yml, and having more control over visibility of the button, the repo it points to, etc, but I think those are OK to wait for a future iteration. Here are a few specific comments in case you have cycles for an iteration on this, but I don't think they need to block it Add documentation somewhereFirst off, I'd like to see some kind of documentation for what users should expect from this feature. This PR is all implementation, and it makes it hard to orient yourself to the expected UX. I know that this repository doesn't have any docs in it, so perhaps you can open a sister PR in mystmd (as a follow up if necessary) to document a basic workflow. Is the Binder URL generation something we can re-use internally?Let's say that we wanted to offer the same launch button-style functionality, but with a gallery of reports / notebooks rather than via the launch buttons. From an implementation standpoint it would be useful if we have a function that takes a page's path and a myst.yml config (or subset of config) and generates the link. That way we could re-use this function in other contexts. Call it a BindeHub URL, or a Binder Server URL, not a Binder URLThis will avoid confusion between the Binder link that a user is copying and the BinderHub server that will build and launch the link. Vertical line between JupyterHub / Binder ?It might help if there were a lightweight vertical line that separated the tabs, to more naturally suggest that they are tabs and not links. Something like this: Auto-add https:// if it's not givenThis might be too much magic, but my first thought was to just type mybinder.org, but I got an error saying it wasn't a valid URL. My feeling is that if the URL doesn't start with a Icon spacing feels a bit weirdThe icon spacing feels a little bit "off" to me. It seems to be in two groups, and the space between groups is bigger than the space within groups. Because there's no visual indicator that there are two groups, the "wide" space between them feels "off" to me. Document that a local preview needs to include
|
@choldgraf thanks!
We can't yet pull a JupyterHub URL because there's no frontmatter entry. I'm holding off on that because it's due a rework. Is this the missing config you're referring to? The rest comes from Binder settings. Otherwise, I've actioned your point RE README, and vertical separator. The other points are hard to do in this pass for various reasons (mainly framework).
I envisage this like a custom role that accepts an xref, e.g. {launch}`xref:references/page` or perhaps annotating an xref That would be cool! |
OK, I took another pass at the config, because I wasn't happy with how it was built out. I briefly explored integrating this more closely with the existing Jupyter infrastructure ... but I think that needs a rework and therefore we're better avoiding making it more complex. Now, this PR pulls from the project binder settings, just like the thebe button does. The JupyterHub support has some nuances:
The latter is a technical limitation with |
Nice - we can upstream some functionality into nbgitpuller if need be, but I agree that's not critical for an MVP. That role to generate a launch link is super cool! I love it. Also makes me wish we had parameters for roles so that you could specify things like the hub URL etc to over-ride the defaults. |
To close jupyter-book/mystmd#1643 and jupyter-book/mystmd#1644, this PR prototypes a launch UI. We will want to move things around in time, but this PR is designed to start design conversations.
There are some deeper conversations around environment provisioning, and execution strategies. They should be ignored for the purpose of this MVP.
As such, this PR
HEAD
Open questions
Next Steps