Conversation
|
|
agoose77
force-pushed
the
agoose77/feat-jupyterlab-v4
branch
from
696e3fd to
402eba1
Compare
|
@agoose77 - I'm very happy to see this progress, feel free to ping me if at some point you want some testing/feedback I can help with. Happy to do it, as having this entire toolchain happily spinning on JL4 is important to me. And huge thanks for the amazing work, once more :) |
|
Ok, I have taken for a spin -- a few things to note is that the new Things I noticed:
Things that worked:
I have also noticed the double render, if you have a handle on that it would be awesome to fix! :) |
|
Thanks @rowanc1! I was actually trying to set up a dev environment to test it, but I got mired in problems with conda/mamba... Argh... In any case, it seems like your round of input may give enough for now - I'll finish cleaning up my local setup so I can help with a round of testing after more updates. If the yarn/jlpm fixes go in that would be great - I know so little about js/ts development that I'm likely to stumble hard on that if it doesn't work out of the box as per the README (which is what I was planning on blindly following). |
There was a problem hiding this comment.
A few of the cells in the example notebook have been removed. Is this intentional?
One of the matplotlib examples does actually fail now, this could be related to the double render, so I think we should fix that first!
I updated trust in #150.
src/TextModelProvider.tsx
Outdated
| model?: ITextModel; | ||
| }; | ||
|
|
||
| const TextModelContext = createContext<TextModelState | undefined>(undefined); |
There was a problem hiding this comment.
Does this mean that task lists work in the markdown preview as well?
There was a problem hiding this comment.
Yes & no. RenderMime isn't supposed to write back to the model IIRC, but we could probably do it if the model we're given is directly associated with the file content, i.e. not a copy. If this were the case, we would need to do a bit more here, though.
In the current version, fragmentContext is set for notebooks only. Perhaps there's a more general refactor to be done, though.
|
I am also in the process of putting together a jlab4-based env for next year courses starting in September in order to try out this wip I was expecting to be able to do which is a recipe that I've used successfully with the jupytext wip at least however in the case of is this easily fixable ? in any case, thanks for taking care of the upgrade 👍 the end of the error log |
* chore: simplify dependencies * fix: update source-map-loader Addresses error with nth-check source-map parsing: webpack-contrib/source-map-loader#186 * chore: upgrade to PyPI version
This is needed for a different PR! oops
…140) * 🎨 Update for showing proofs * Update Playwright Snapshots --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SHA256 hashes: jupyterlab-myst-1.2.0.tgz: e730003c864ec43b85b1d2f0f8fe2cc9ad318a9d9caaa48348b055d2ee85f9d0 jupyterlab_myst-1.2.0-py3-none-any.whl: 29b8055003e713058f9702fcf8d28fbbf74002c9e66cf4625d8746d7ee8f22ae jupyterlab_myst-1.2.0.tar.gz: fbc686a79543cdd439d256c71d26410d281bcdff6620ff92b7fea247a71b6f12
agoose77
force-pushed
the
agoose77/feat-jupyterlab-v4
branch
from
af05b3f to
f0c30a6
Compare
|
Turns out that the screenshot action is based off of main, and that is why there was a failure and the updates to the YAML were not working. Pretty unintuitive, but should be fine for anything except a jupyterlab release. |
rowanc1
left a comment
rowanc1
left a comment
There was a problem hiding this comment.
Final look through the code, I think all this is great and super happy to get this in and have a major release to support jlab 4. I think some of the things that happen right after that are the double render improvements and then try to get to some of the other major features that we need!
Nice work @agoose77 🚀
| @@ -1,2841 +0,0 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
Not sure if we want to bring this yarn-lock back, or we could add it to the gitignore, it is probably fine for it to be generated every time.
|
Ok, fixed the wrong cell model being shown -- in my JupyterLab (or notebook?) the Right now there are a lot of errors being thrown, I think that these should be at most debug messages as we are actually expecting them to fail. Having this be the default state of the console is a bit rough - also not sure if we should leave the console logs in, but if we do, we should make sure that they are
|
| // Create renderer | ||
| const renderer = this.rendermime.createRenderer(mimeType); | ||
| layout.widget = renderer; | ||
| if (layout) layout.widget = renderer; |
There was a problem hiding this comment.
I am not sure why, but this was consistently null.
|
Hmm, I've not been seeing that. In any case, I'm not happy about the PR as it stands - after our meeting I took another look and I have a hunch that we can revert back to something more like our original code (main). I know that we discussed releasing as is, but I've made good progress, I'll see tomorrow if it works and then we can feel more confident moving forward. I'll check out this bug as a part of that. |
rowanc1
left a comment
rowanc1
left a comment
There was a problem hiding this comment.
Ok, extension is working again with my minor changes! Without the double render is MUCH nicer from a UX perspective. Would prefer not to have the console errors when we release this, but isn't a big deal and can always be fixed later.
| const renderer = this.rendermime.createRenderer(mimeType); | ||
| layout.widget = renderer; | ||
| if (layout) layout.widget = renderer; | ||
| console.assert(renderer.isAttached, 'renderer was not attached!', renderer); |
There was a problem hiding this comment.
Many of these calls consistently fail, and aren't actually a problem. I think that we should not show console errors unless there is a problem. We could also put them behind a dev flag if we want.
|
Not sure, maybe this will just go away .. says it is a github internal error: |
|
Thanks for the pip line @parmentelat! Indeed, for someone like me with such limited JS/TS expertise, being able to test like this is pretty important :) Here just to report that things are looking pretty good so far on a 4.0.2 installation, thanks so much @agoose77 @rowanc1!! I am not sure if the math double rendering is entirely gone or just less noticeable due to performance improvements, but in any case the experience with inline math is much, much smoother and more reliable than very recently. It was very hit or miss in my prior tests (I always had to demo it with a caveat to the audience :) and now it works pretty well (in light testing though). I noticed that inline markdown links are not properly formatted, though they do work as URLs. See e.g. the example for the youtube link in the included example:
That was already probably on your radar, but just in case. As usual thanks so much for the great work, LMK if I can be of any help with further testing/input. Very much looking forward to this being fully released! |
|
Taken over by #155 |
5 participants
This PR will probably take a while. The new windowed notebook breaks things that will require some care to recover.
Simultaneously, I think we should think about how #102 fits into this PR at the same time.
Once this gets merged, it might be more straightforward for us to only support 4.x. I haven't had much experience in maintaining compatibility across multiple versions. The obvious way is to make a branch from
HEAD~1after merging this PR, and bump the version inmainto2.0.0. Then we can continue to make backports if needs be.