Readme viewer

[toonijn]

A first draft to add a view of the readme in a directory. As suggested in #2485 (comment).

[toonijn]

Can I get a review on this?

[kevin-bates]

Hi @toonijn - Sorry for the delay. I don't have front-end expertise but thought I'd build your branch and try it out from a functional perspective. However, when I build/run your branch, I see the following...

Is there anything I need to enable to get this to display correctly?

As a general comment on the PR, I'm wondering why we'd constrain markdown views to just readme.md files and not all markdown files in general?

[toonijn]

The markdown preview is added to the tree view. So editing markdown files is still the same.

If you save the file you are editing in your screenshot, then go back to the filetree, you should see in the directory of the file the preview of that readme.md. It's like the readme-file in a github repo.

Can you confirm this?

[kevin-bates]

Hmm. I had started with a README.md file (from GH), but I just tried creating a new text, renamed untitled.txt to README.md, added some markdown, saved the file, and still don't see the markdown rendered anywhere. I guess my issue is there's no "preview".

How is preview enabled? Is this handled by some extension?

[toonijn]

@kevin-bates It should be default enabled. It should show up in your screenshot. Just a sanity check: did you run npm run build?

[kevin-bates]

Thanks for that npm tip. However, I'm still not seeing the preview. Here are the steps I used...

• git clean -xfd
• npm run build
• pip install -e . (Same results as building the whl and installing that.)
• jupyter notebook --debug --notebook-dir=~/notebooks

I've tried chrome, firefox and safari, regular and private windows. Since I'm unable to provide a code review anyway, I'm not sure how useful my being able to run this really is - although it would allow me to provide a functional review.

[toonijn]

First of all, thank you for taking the time to try it out.

I have reproduced the issue on Windows. On my linux everything was working as expected. The problem was that windows does not specify the mimetype of a file (I removed a check that only displayed text/markdown mimetypes).

@kevin-bates a functional review would be very useful feedback. Looking forward to your comments!

[kevin-bates]

Cool - with this last commit, I'm able to preview readme.md files in various directories under my (notebook) root. I'm running this on a mac, so it must have a similar issue that Windows had.

This does seem useful. I'm just not sure if it's too specific. Also, if I have readme.txt, it will be previewed. Seems like the file-type check was useful.

[toonijn]

The reason why I wanted to implement this is to use it in our university's JupyterHub server. If a student logs in to their server and uses Jupyter for the first time, it would be very helpful to post a small specific quick-start guide. But I can see multiple uses:

• For a JupyterHub server with multiple first-time users of jupyter. (Like in my case)
• If you're sharing projects , the readme could describe all the different notebooks
• If you're mounted to a shared filesystem to describe information about different directories

I agree for single-user situations it maybe isn't that much of an advantage. But in the multi-user case it would be much appreciated.

You bring up a good point, about readme.txt getting shown as well. But why shouldn't it? Which files should be displayed, and which not? This gets even harder to answer because files don't really have a 'type', the best guess is the extension. Do you have suggestions?

[kevin-bates]

I agree with your use cases - especially those in more collaborative or replicated (edu) environments.

You're right about the file type determination, suffix tends to be the first order of determination. When I have both readme.md and readme.txt present, it seems like the markdown file takes precedence. This seems good but I don't know if that's just dumb luck. However, if there's a way to force markdowns to take priority - that would be good. We need to avoid random behavior.

If I delete the previewed file, the preview remains until the next refresh even though the tree reflects the current state. Is there a way to hook the preview into the tree refresh?

[toonijn]

@kevin-bates

The updates to the readme are triggered by a 'draw_notebook_list.NotebookList'-event. But deleting a file doesn't seem to trigger that event. I've fixed it by listening to the 'delete'-event as well.

The render order I propose is:

1. 'readme.md' or 'readme.markdown' (rendered as markdown)
2. 'about.md' or 'about.markdown' (rendered as markdown)
3. 'readme' or 'readme.*' (rendered as .txt)
4. 'about' or 'about.*' (rendered as .txt)

When a file is rendered as markdown it looks like in the first comment on this PR.
Rendering as .txt looks like:

[kevin-bates]

Thanks for the update Toon! Unfortunately, this will need a review performed by one of the front-end-knowledgable maintainers at this point. I've requested reviews based on the suggestions GH provides.

[gnestor]

Hi @kevin-bates, thanks for your contribution! Unfortunately, I'm not available to review right now ☹️

[Zsailer]

We'll discuss this PR at the Notebook Meeting this Wednesday (30 minutes before the JupyterLab meeting).

This is really great work, @toonijn, and greatly appreciated! Thank you for devoting a significant amount of time to this.

This is a pretty significant chunk of code and it adds a "new feature" in Jupyter Notebook. We've been avoiding accepting "new features" since we don't have any people available to maintain + review these kinds of additions (most of our dev time has been focused on JupyterLab; but the messaging around this hasn't exactly been clear).

We'll try to give this a solid review on Wednesday, but I can't promise that we'll merge this in the end. We're stretched a bit too thin to support many new features. In the meantime, you can, of course, offer this as a notebook extension.

Either way, I'll update you on Wednesday with our decision here. Thanks, again!

[n08u]

FYI, we have similar features implemented as an extension: https://github.com/NII-cloud-operation/Jupyter-LC_index . This one puts README.md at the bottom and also puts README.svg on the top. One unresolved drawback is that this extension trusts SVG blindly.

[toonijn]

Thanks for considering.

Small note: a large part of the changed files is a minor refactor to bring all markdown-renders in the same code. (notebook/static/base/js/markdown.js)

[Zsailer]

@toonijn Thanks for your work here.

Let me give you an update—we met last week to discuss the current state of jupyter/notebook and various PRs adding new features (this PR was at the top of the list). In short, our plan is to not accept new features into the core classic Jupyter Notebook frontend. Unfortunately, we just don't have the bandwidth on our core development team to support new features here. Instead, we are going to encourage contributors to create Notebook extensions when they want to offer new frontend features to the community. We are working on adding this message to the README, issue templates and PR templates from this point forward.

For this PR, there are two things to discuss:

1. We won't be adding the README viewer to the core Notebook frontend. We all agree this feature is very useful and would make a great extension, but we avoiding supporting new features as Jupyter Notebook moves into a state of LTS. Perhaps you can merge this with @n08u's nbextension?
2. The refactoring you did to eliminate duplicate code in the markdown renderers is very helpful. We'd love to review+accept a PR with this refactoring left in place. If you plan to open a separate PR with that logic, ping me for review and we'll get that merged ASAP.

I really appreciate all the work you put in here, @toonijn. I'm sorry we couldn't have a more positive message here.

[Zsailer]

I'm closing this for now, but let me know if you're still interested in submitted the code duplication fix. I'm happy to review that work in another PR. Thanks!

[fbe555]

Any updates of this very useful piece of work becoming available as an extension ? :)

[toonijn]

@fbe555
For our production server we switched to Jupyter Lab. So I did not take the time to make an extension out of it. But https://github.com/NII-cloud-operation/Jupyter-LC_index seems to implement the same functionality.