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

Use our own jinja2 template loader #255

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Conversation

yuvipanda
Copy link
Contributor

  • Our template loader setup was a hack, and that hack seems to
    have stopped working.
  • We just use our own loader instead. This means we lose the base
    translation functionality of upstream render_template - but we
    were not using it at all anyway, so 'tis alright.
  • Looks like the path for the stylesheet we were 'borrowing'
    from classic notebook changed location, so update to use the
    new location. This will stop existing after notebook v7,
    and we will need to use our own.

Fixes #235

- Our template loader setup was a hack, and that hack seems to
  have stopped working.
- We just use our own loader instead. This means we lose the base
  translation functionality of upstream render_template - but we
  were not using it at all anyway, so 'tis alright.
- Looks like the path for the stylesheet we were 'borrowing'
  from classic notebook changed location, so update to use the
  new location. This will stop existing after notebook v7,
  and we will need to use our own.

Fixes jupyterhub#235
@yuvipanda
Copy link
Contributor Author

@manics this is an alternative to #240. We need to do #240 as well regardless, but I don't understand the implications enough.

/cc @Zsailer who helped point this out.

@yuvipanda
Copy link
Contributor Author

@betolink you might be interested in this too.

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This feels like a cleaner implementation, and since it fixes a nasty bug I think this is a great improvement! +1 from me unless somebody uncovers any other technical issues

@yuvipanda
Copy link
Contributor Author

I think #240 is also needed, but @Zsailer told me more is needed to turn it into a proper jupyter-server application. I don't know enough about what more needs to be done there though.

@Zsailer
Copy link
Member

Zsailer commented Mar 16, 2022

Can someone here make it to one our Jupyter Server Contributor Hours? 😃

It would be really fun to try and port this extension in real-time. Then, we can process record, document, and share this in the server docs. I'm happy to lead the development and we can pair program as a team during the call.

@choldgraf
Copy link
Member

Just to be clear @Zsailer - you're talking about the resolution of this issue, right?

Agree that would be great :-) but I'm still +1 on merging this PR in the meantime!

@Zsailer
Copy link
Member

Zsailer commented Mar 17, 2022

Ah, yes—I was referring to #240. Sorry for the confusion! 😃

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Thanks!

@manics manics merged commit 6c0602b into jupyterhub:main Mar 17, 2022
@yuvipanda
Copy link
Contributor Author

Thanks a lot, @manics! Do you think we can do a release?

@manics
Copy link
Member

manics commented Mar 18, 2022

I think so, if we don't need anything from #240

@yuvipanda yuvipanda added the bug label Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatible with jupyter-labhub
4 participants