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.

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

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@
"node-fetch": "^2.7.0",
"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
108 changes: 104 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ __metadata:
languageName: node
linkType: hard

"@asamuzakjp/dom-selector@npm:^2.0.1":
version: 2.0.2
resolution: "@asamuzakjp/dom-selector@npm:2.0.2"
dependencies:
bidi-js: ^1.0.3
css-tree: ^2.3.1
is-potential-custom-element-name: ^1.0.1
checksum: a454537fcba4f241d3c1303f6068944462fc0ba9cd2c5e3ad639c0acb58ffb7809e5d4cbdac805c8c2525b2450d53a992ff98f07a323c5246044e8e3de3561fe
languageName: node
linkType: hard

"@aws-crypto/crc32@npm:3.0.0":
version: 3.0.0
resolution: "@aws-crypto/crc32@npm:3.0.0"
Expand Down Expand Up @@ -3515,6 +3526,7 @@ __metadata:
node-fetch: ^2.7.0
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 Expand Up @@ -6270,6 +6282,15 @@ __metadata:
languageName: node
linkType: hard

"bidi-js@npm:^1.0.3":
version: 1.0.3
resolution: "bidi-js@npm:1.0.3"
dependencies:
require-from-string: ^2.0.2
checksum: 877c5dcfd69a35fd30fee9e49a03faf205a7a4cd04a38af7648974a659cab7b1cd51fa881d7957c07bd1fc5adf22b90a56da3617bb0885ee69d58ff41117658c
languageName: node
linkType: hard

"big-integer@npm:^1.6.17":
version: 1.6.51
resolution: "big-integer@npm:1.6.51"
Expand Down Expand Up @@ -7425,6 +7446,16 @@ __metadata:
languageName: node
linkType: hard

"css-tree@npm:^2.3.1":
version: 2.3.1
resolution: "css-tree@npm:2.3.1"
dependencies:
mdn-data: 2.0.30
source-map-js: ^1.0.1
checksum: 493cc24b5c22b05ee5314b8a0d72d8a5869491c1458017ae5ed75aeb6c3596637dbe1b11dac2548974624adec9f7a1f3a6cf40593dc1f9185eb0e8279543fbc0
languageName: node
linkType: hard

"css-what@npm:^6.1.0":
version: 6.1.0
resolution: "css-what@npm:6.1.0"
Expand Down Expand Up @@ -8282,10 +8313,10 @@ __metadata:
languageName: node
linkType: hard

"dompurify@npm:^3.0.11":
version: 3.0.11
resolution: "dompurify@npm:3.0.11"
checksum: aefb86fbaa2cc6acda1a75ea918f69043689cb726f2932e5c1c3c85904255fd145230d66f0a9493ed27d64d035e990f3a767d99d73b1ae7c0b297782be230658
"dompurify@npm:^3.0.11, dompurify@npm:^3.0.8":
version: 3.1.2
resolution: "dompurify@npm:3.1.2"
checksum: 450edfacc3918db29afb417a9f9d3fcb00412fe33435eb809b087f746b75c3d50b8e2520fac67efeef249664eba3de8524a0355172ec22eb151157aece3edf31
languageName: node
linkType: hard

Expand Down Expand Up @@ -11413,6 +11444,40 @@ __metadata:
languageName: node
linkType: hard

"jsdom@npm:^23.2.0":
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unfortunate that this pulls in a version of jsdom that's effectively unused; this leaves us with an extra 4MB of dependencies to ship around. I'm wondering if we could resolve this with Yarn resolutions: https://yarnpkg.com/configuration/manifest#resolutions

I'm happy to try out this change and push it to your branch if that's OK with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with that.
I'll fix the first two issues Tuesday or Wednesday.

version: 23.2.0
resolution: "jsdom@npm:23.2.0"
dependencies:
"@asamuzakjp/dom-selector": ^2.0.1
cssstyle: ^4.0.1
data-urls: ^5.0.0
decimal.js: ^10.4.3
form-data: ^4.0.0
html-encoding-sniffer: ^4.0.0
http-proxy-agent: ^7.0.0
https-proxy-agent: ^7.0.2
is-potential-custom-element-name: ^1.0.1
parse5: ^7.1.2
rrweb-cssom: ^0.6.0
saxes: ^6.0.0
symbol-tree: ^3.2.4
tough-cookie: ^4.1.3
w3c-xmlserializer: ^5.0.0
webidl-conversions: ^7.0.0
whatwg-encoding: ^3.1.1
whatwg-mimetype: ^4.0.0
whatwg-url: ^14.0.0
ws: ^8.16.0
xml-name-validator: ^5.0.0
peerDependencies:
canvas: ^2.11.2
peerDependenciesMeta:
canvas:
optional: true
checksum: 3ba97e6ac56c38d92d0ce2d0fac5de4042f7dec40d127872e1aa88dd379980f8ea2108a008319ceac54dc07a784078ed4b4401bf9109a76276ca2cace229c8df
languageName: node
linkType: hard

"jsdom@npm:^24.0.0":
version: 24.0.0
resolution: "jsdom@npm:24.0.0"
Expand Down Expand Up @@ -12067,6 +12132,15 @@ __metadata:
languageName: node
linkType: hard

"marked@npm:^11.1.1":
version: 11.2.0
resolution: "marked@npm:11.2.0"
bin:
marked: bin/marked.js
checksum: 80757c268420d58d2fb2dadb8e1f9e80b96160dc667de69fe455cf5fd874e5cfec87a171249f49f28729e90de4bafa1f586562ffaad04836fe6e6c355365f60a
languageName: node
linkType: hard

"mathjax@npm:^3.2.2":
version: 3.2.2
resolution: "mathjax@npm:3.2.2"
Expand Down Expand Up @@ -12218,6 +12292,13 @@ __metadata:
languageName: node
linkType: hard

"mdn-data@npm:2.0.30":
version: 2.0.30
resolution: "mdn-data@npm:2.0.30"
checksum: d6ac5ac7439a1607df44b22738ecf83f48e66a0874e4482d6424a61c52da5cde5750f1d1229b6f5fa1b80a492be89465390da685b11f97d62b8adcc6e88189aa
languageName: node
linkType: hard

"mdurl@npm:^1.0.0":
version: 1.0.1
resolution: "mdurl@npm:1.0.1"
Expand Down Expand Up @@ -13017,6 +13098,18 @@ __metadata:
languageName: node
linkType: hard

"notebookjs@npm:^0.8.2":
version: 0.8.2
resolution: "notebookjs@npm:0.8.2"
dependencies:
ansi_up: ^6.0.2
dompurify: ^3.0.8
jsdom: ^23.2.0
marked: ^11.1.1
checksum: 540ed818cc2ad55efe1281250c9797afd6f44152d9d271a52a7dfb6ead2b5a2ef87de56691126dbcd992dc2c1b3ba4eb3dd49c28f1a85bda1c050e946a0e0985
languageName: node
linkType: hard

"notepack.io@npm:~3.0.1":
version: 3.0.1
resolution: "notepack.io@npm:3.0.1"
Expand Down Expand Up @@ -15398,6 +15491,13 @@ __metadata:
languageName: node
linkType: hard

"source-map-js@npm:^1.0.1":
version: 1.2.0
resolution: "source-map-js@npm:1.2.0"
checksum: 791a43306d9223792e84293b00458bf102a8946e7188f3db0e4e22d8d530b5f80a4ce468eb5ec0bf585443ad55ebbd630bf379c98db0b1f317fd902500217f97
languageName: node
linkType: hard

"source-map-support@npm:^0.5.21":
version: 0.5.21
resolution: "source-map-support@npm:0.5.21"
Expand Down
Loading