fixing bug #134
fixing bug #134
Conversation
| app.add_javascript("link_gen/link.js") | ||
| app.add_javascript("link_gen/vendor/bootstrap-4.1.3/js/bootstrap.bundle.min.js") |
There was a problem hiding this comment.
Is it safe to delete these files from the link_gen/vendor/ folder? https://github.com/jupyterhub/nbgitpuller/tree/master/docs/_static/link_gen/vendor
|
good point - just removed those files (and also an extra "onFormChange" call that I think is outdated |
There was a problem hiding this comment.
I tested this too and it looks like the link gets generated for JupyterHub and Canvas with this change even when content-repo is not provided and the onFormChange error doesn't appear anymore in console 🎉
This also happens though:
It needs input to generate the link when switching tabs from JupyterHub to Canvas. Maybe we can call the displaylink on tab changes too?
Also, sorry for introducing this bug 😢
|
Does anyone have experience with writing JavaScript tests? It might be a good addition for a future PR. Although this is a relatively simple JS app the tests could also serve as a good tutorial for inexperienced Javascript testers. |
|
@GeorgianaElena I kinda think it's OK to ask users to take another action when they switch tabs. My guess is that the large majority of users will do:
This is more likely now that the tabs can be activated with a REST url too. So while we can definitely improve this behavior, I don't know that it needs to block this PR (since this PR fixes a more prominent bug). What do you think? @manics I have intentionally avoided learning how to write javascript tests because I want to keep a plausible reason to never write them :-) |
|
It makes sense, @choldgraf. Thanks for explaining. This looks LGTM then 🚀 |
|
@manics are you OK with me merging this? (or do you wanna merge?) I don't wanna jump-start it since you've commented on the PR :-) |
I believe this resolves #130
It does the following:
content-repoform field optional for the non-binder tabstry it here: https://119-92329878-gh.circle-artifacts.com/0/html/link.html