-
Notifications
You must be signed in to change notification settings - Fork 504
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
Lab theme handling #1089
Lab theme handling #1089
Conversation
af471b4
to
6b3c68e
Compare
I need to update the visual regression test references (due to nbconvert change). Will wait for the bot to be added in #1083. |
Hi @martinRenou, if I understand correctly jupyter/nbconvert#1703, the content of the CSS files will be embedded directly into the generated HTML, so why in |
One of the advantages of properly serving the theme assets (CSS, images, fonts) is that it allows the browser to cache them. While if we embed them in the HTML the browser will not be able to cache them. In the case of nbconvert we want the resulting HTML file to be portable (we want to be able to render the HTML file without having to look for the CSS assets somewhere else), that's why embedding makes sense. But in the case of Voila we have a proper server, so we should probably use it. |
Also, adding the |
Thanks for the explanation! I forgot about the caching, it totally makes sense to serve assets via URL. |
Sure! :) I should probably add a visual regression test to this PR |
Indeed, I think visual regression tests are needed for this kind of change. |
6b3c68e
to
5dcb7f3
Compare
Update galata references |
4496a23
to
c611eae
Compare
Update galata references |
Added support for query parameters on the TreeHandler, it will keep the query when changing directory and rendering Notebooks: query.mp4I needed this to be able to test the theme handling in the Tree view, and I guess it's a nice addition? |
223d6d6
to
46d0263
Compare
Update galata references |
triggering CI |
Benchmark reportThe execution time (in milliseconds) are grouped by test file, test type and browser. Results table
❗ Test metadata have changed--- /dev/fd/63 2022-02-11 13:38:58.373779974 +0000
+++ /dev/fd/62 2022-02-11 13:38:58.373779974 +0000
@@ -4,37 +4,37 @@
"BENCHMARK_REFERENCE": "actual"
},
"browsers": {
- "chromium": "97.0.4666.0"
+ "chromium": "94.0.4595.0"
},
"systemInformation": {
"cpu": {
- "brand": "Xeon® Platinum 8272CL",
+ "brand": "Xeon® E5-2673 v3",
"cache": {
"l1d": 65536,
"l1i": 65536,
- "l2": 2097152,
- "l3": 36700160
+ "l2": 524288,
+ "l3": 31457280
},
"cores": 2,
"family": "6",
- "flags": "fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch invpcid_single pti fsgsbase bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx avx512f avx512dq rdseed adx smap clflushopt avx512cd avx512bw avx512vl xsaveopt xsavec xsaves md_clear",
+ "flags": "fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm invpcid_single pti fsgsbase bmi1 avx2 smep bmi2 erms invpcid xsaveopt md_clear",
"governor": "",
"manufacturer": "Intel®",
- "model": "85",
+ "model": "63",
"physicalCores": 2,
"processors": 1,
"revision": "",
"socket": "",
- "speed": 2.6,
+ "speed": 2.4,
"speedMax": null,
"speedMin": null,
- "stepping": "7",
+ "stepping": "2",
"vendor": "GenuineIntel",
"virtualization": false,
"voltage": ""
},
"mem": {
- "total": 7284850688
+ "total": 7291699200
},
"osInfo": {
"arch": "x64",
@@ -42,11 +42,11 @@
"codename": "Focal Fossa",
"codepage": "UTF-8",
"distro": "Ubuntu",
- "kernel": "5.11.0-1028-azure",
+ "kernel": "5.8.0-1040-azure",
"logofile": "ubuntu",
"platform": "linux",
"release": "20.04.3 LTS",
- "serial": "1b14fffe90ff4198b51ddebf6333f628",
+ "serial": "cfc067bfcb844f35865e279a1b0e66c5",
"servicepack": "",
"uefi": false
} |
14b619c
to
23a4dfb
Compare
23a4dfb
to
61c6466
Compare
61c6466
to
45caae8
Compare
@@ -76,7 +87,8 @@ def allowed_content(content): | |||
contents=contents, | |||
terminals_available=False, | |||
server_root=get_server_root_dir(self.settings), | |||
theme=self.voila_configuration.theme, | |||
theme=theme_arg, | |||
query=self.request.query, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should pass all queries of the tree page to the notebook URLs, maybe just the theme and template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of this PR, that's the only two query arguments we support, or do we support more arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.request.query
contains all the queries passed to the tree handler, and then it is appended to the notebook URLs. I'm thinking about a more limited scope where only voila-theme
and voila-template
are transferred to the URL
Updated: it might be nicer to pass all queries, it'll allow providing configurations for all notebooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your original comment will make sense if we add some query arguments specific to the TreeHandler at some point. But we can probably filter the query arguments here only when that happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Voila
is served under proxy, there might be some sort of token query for the tree handler, but I think there is no harm in transferring these tokens to the notebook URLs. We can filter the query when we actually use them in the tree handler.
Thanks @martinRenou |
References
Builds on top of jupyter/nbconvert#1703
Code changes
Add handlers for the JupyterLab federated extension themes. This adds a dependency on
jupyterlab_server
which provides theThemesHandler
.User-facing changes
miami.mp4
Backwards-incompatible changes
None