Closed rowanc1 closed 1 year ago
We can get the JupyterLab sanitiser from the renderer options: https://jupyterlab.readthedocs.io/en/stable/api/interfaces/rendermime.irendermime.irendereroptions.html
It looks like the current renderer doesn't expose a mechanism for passing additional arguments. I think that we probably want the sanitisation to happen in the src/renderers.tsx
file rather than earlier in the AST building (for obvious reasons), so is there a way to extend this?
I am also noticing that .jp-RenderedHTMLCommon
is required on the markdown cell for the alert to work. I did not add that as it changes links and quite a few other things that we don't want. This is going to take some other style work. 🤔
We can also do this much easier (at AST parsing/transformation time) in that place I think we have access to all of the various things we need. We will also have to merge the HTML tokens and sanitize them together.
Hello!
I am starting to plan for our JupyterHub and course upgrades for next Semester (early September). No pressure intended, but do you have some rough timeline for this PR?
Thanks in advance,
Hi @rowanc1,
I am preparing the upgrade of our jupyterhub for the upcoming teaching semester starting September 11th and am facing a dilemma:
Depending on the tentative timeline for this PR, do you have a recommandation for which route to take?
Thanks in advance,
@nthiery we can definitely get a release of the old jupyterlab-myst
that fixes the jupyter_server
limit (although you should be safe to forcibly ignore this), but I'd hope we can move you to the latest. Are there any limitations on the version of JupyterLab you can support?
Meanwhile, I'll let Rowan speak to the HTML side of things.
Thanks @agoose77 for the quick feedback! We will be using jupyterlab 4.*.
Can you confirm whether you use inline execution, or is it just MyST rendering?
@agoose77 We just use MyST rendering, no inline expressions (I assume that's what you meant with inline executions).
Just as a data point: I made a minimal install of JupyterLab 4
and jupyterlab-myst 0.1.7a6
;
markdown cells are then rendered as empty.
Maintaining the 0.1 line is probably a waste at this stage; hoping for another solution to be in reach ...
environment:
name: jupyterhub-paris-saclay-test
channels:
- conda-forge
dependencies:
- jupyterlab>=4
- jupytext
- pip
- pip:
- jupyterlab-myst==0.1.7a6 # Workaround latest mystjs-based implementation not preserving HTML admonitions
- .
I've updated this PR with a couple things. Rowan already removed htmlPlugin
, which previously converted all html to mdast, and just lost any unsupported html element type, e.g. button. Now, the mdast is also passed through a new transform that resolves broken-up, inline html (referred to in Rowan's original comment on this PR, see https://github.com/executablebooks/mystmd/pull/575). We also need to add the jupyter html sanitization (I added a separate library to sanitize, which probably shouldn't be landed; it pulls in jsdom
... too big!)
@agoose77 - I'll spend a few more minutes to see if I can sort out the sanitizer from jupyter. Then once the myst PR is landed, I'll kick it over to you to look over! Let me know if you think there are any aspects of this I'm totally overlooking...
This sounds very promising! Let me know if beta-testing can help at any point. I am your man :-)
@fwkoch thanks a bunch for this! I installed a local build of mystmd, and I can see that the inline styles work. The default styling for buttons isn't brilliant - could you take a look at that? I don't know what we're aiming for, but what's there right now doesn't look button-like at all.
I've updated the PR to the changes in main
, which brings in the latest MyST-md work.
Another point - could we move myst-theme
to 1.x
versioning? The existing caret constraint in jupyterlab-myst
is too restrictive because myst-theme
is on 0.x
@agoose77 - thanks for jumping into the sanitizer stuff! I played around with simply importing the default sanitizer
from @jupyterlab/apputils
(instead of dompurify
) - but it looks like your solution to inject the sanitizer is much better (and still defaults to that sanitizer probably...?).
Anyway - the new version of myst-transforms
(and other myst libs) has been released, so you should be able to update away from your local install.
Regarding myst-theme
version I don't really see a problem, @stevejpurves do you have any concerns there?
And as for button styling, I don't have any opinions or experience... Does the default styling match how things used to look in the previous markdown renderer...? If so, maybe it's just good enough, and we can do better-looking MyST buttons
in another pass? But maybe it's more than just buttons, e.g. Rowan's comment above about .jp-RenderedHTMLCommon
; I have no feeling about what to do there. Maybe you have some opinions on this too @stevejpurves ? 🙂
Tailwind seems to strip away the button styling, but I don't see any CSS in our stylesheets to restore anything. I assume the expectation is that we add the appropriate tailwind classes, but we'd want to do this ourselves in the MyST stylesheet I think.
Also, I didn't notice that I committed my local paths. Whoops!
We can move myst-theme
to 1.0 🥳 whenever we like. @agoose77 sorry I'm catching up, but I didn't really understand the issue you were having with the ^0.x
caret
There's nothing inherently wrong with ^0.x
, but it does differ in semantics to ^N.x
(N != 0
). It wasn't actually this PR that I noticed the problem though, rather this commit where I had to manually bump the constraint because we expect 0 to mean major-version 0.
Also on tailwind, yet is does strip away all base styles. I note some styling of button here https://github.com/executablebooks/jupyterlab-myst/blob/main/style/preflight.css but am not clear on the meaning of preflight
?
In any case we can introduce anywhere and apply some basic styles, maybe something like:
.myst button {
@apply bg-blue-500 hover:bg-blue-700 text-white font-bold py-2 px-4 rounded
}
But adjusting those colors to fit
My understanding of preflight is that it's a tailwind concept to introduce a clean slate upon which to customise the styling.
@fwkoch @stevejpurves I thought I'd already written this message but couldn't find it — it's proving tricky to correct the button styles because it transpires that the authors
card renders each author as a button. Is there a good reason for this vs an <a>
link?
@nthiery I'm really conscious that your course is starting soon (tomorrow?). Could you perhaps expand upon whether you'll have <button>
elements that we need to fix the styling for?
Hi @agoose77,
I really appreciate all the efforts each of you are putting into this! My course started today, but I found ways to wiggle around: not updating our main server to not break other's teachers work; having my own course notes a bit degraded. In short, I can hold my breath a bit more.
If in the current state it's working for most elements, maybe you could cut a beta release, and I am happy to deploy it and have it tested at "large scale" (in terms of users and content) to provide you feedback.
This isn't "done", but I think an alpha release would be helpful. I'll merge.
Yeah! Will try this tonight :-)
This is an initial sketch of fixing #64.
In JupyterLab, we should be doing HTML rendering directly, but it does need to be sanitized using Jupyter's machinery. @agoose77 maybe you can help here?
@fwkoch it looks like certain inline elements get split up into different html tags, at least when they are inline. Maybe we just need a transform to get these into one html element, then we can render that easily.