Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add preview for .ipynb files in pl-file-preview #9840

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+107 −406
Open

Add preview for .ipynb files in pl-file-preview #9840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22e2636
added notebookjs
kurmasz May 10, 2024
1044993
Merge branch 'PrairieLearn:master' into zk_add_workspace_preview2
kurmasz May 10, 2024
598d4a7
Sync'd with updated master. Verified it works
kurmasz May 10, 2024
41eb257
ran dedup
kurmasz May 10, 2024
f677b14
Merge branch 'master' into zk_add_workspace_preview2
kurmasz May 20, 2024
3ae91a2
resolved conflicts
kurmasz May 20, 2024
f798969
Force jsdom version for dependencies
nwalters512 May 20, 2024
52c3fbe
Fixed merge conflicts with yarn.lock
kurmasz May 20, 2024
410e5c3
Merge branch 'zk_add_workspace_preview2' of github.com:kurmasz/Prairi…
kurmasz May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/prairielearn/elements/pl-file-preview/info.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
{
"controller": "pl-file-preview.py",
"dependencies": {
"nodeModulesScripts": [
"dompurify/dist/purify.min.js",
"marked/marked.min.js",
"notebookjs/notebook.min.js"
Comment on lines +6 to +7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be loading these unconditionally if there is no notebook to preview; that's a waste of CPU/bandwidth. I'd recommend using dynamicDependencies as documented here: https://prairielearn.readthedocs.io/en/latest/devElements/. Then you can import() them only when there is actually a .ipynb file to preview.

],
"elementScripts": ["pl-file-preview.js"],
"elementStyles": ["pl-file-preview.css"]
}
Expand Down
12 changes: 12 additions & 0 deletions apps/prairielearn/elements/pl-file-preview/pl-file-preview.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,15 @@
border-top-right-radius: 0;
border-top-left-radius: 0;
}

.nb-input pre {
color: lightgreen;
}

.nb-markdown-cell p {
line-height: 0;
}

.nb-markdown-cell li {
line-height: 0;
}
20 changes: 17 additions & 3 deletions apps/prairielearn/elements/pl-file-preview/pl-file-preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
/* eslint-env browser,jquery */

/* global nb */
// Note about notebookjs: I'm not thrilled with how notebook and marked are styling the
// Markdown. Unfortunately, I don't have the design skills to make it any better than
// it is right now. Perhaps somebody else can pick up where I left off and make improvements.
// Similarly, it might be nice to have syntax highlighting for the code blocks; but, that is
// also a task for another day.
// One last idea: Do we want a "hide markdown" button (assuming that it is usually the code
// blocks that are of interest to most instructors)?
Comment on lines +3 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking comments that way "I" or "should we" shouldn't be committed. You can rephrase the first paragraph to read something like "The notebook styling is less than ideal, etc. etc.". You can add a // TODO comment for syntax highlighting if you think it's important. I don't think a "hide markdown" button is necessary now.

(() => {
async function downloadFile(path, name) {
const result = await fetch(path, { method: 'GET' });
Expand Down Expand Up @@ -112,8 +119,15 @@
.then(async (blob) => {
const type = blob.type;
if (type === 'text/plain') {
const text = await blob.text();
code.textContent = text;
if (escapedFileName.endsWith('.ipynb')) {
const notebook = nb.parse(JSON.parse(await blob.text()));
const rendered = notebook.render();
pre.appendChild(rendered);
} else {
const text = await blob.text();
code.textContent = text;
}

pre.classList.remove('d-none');
hideErrorMessage();

Expand Down
1 change: 1 addition & 0 deletions apps/prairielearn/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@
"node-fetch": "^3.3.2",
"node-jose": "^2.2.0",
"nodemon": "^3.1.0",
"notebookjs": "^0.8.2",
"numeric": "^1.2.6",
"oauth-signature": "^1.5.0",
"object-hash": "^3.0.0",
Expand Down
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@
"turbo": "^1.13.3",
"typescript": "^5.4.5"
},
"resolutions": {
"notebookjs/jsdom": "^24.0.0",
"fabric/jsdom": "^24.0.0"
},
"workspaces": [
"apps/*",
"packages/*"
Expand Down
Loading
Loading