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

Fix syntax highlighting for JSON viewer in Jupyter Notebook #13470

Conversation

kostyafarber
Copy link
Contributor

References

This PR addresses jupyter/notebook#6567 in Jupyter Notebook, by making an upstream change here in JupyterLab.

Code changes

The changes are to the json-extension which mount jupyterHighlightStyle manually.

User-facing changes

The users of Jupyter Notebook will see the same as what they see in JupyterLab when opening a JSON file.

Before

image

After

image

Backwards-incompatible changes

No anticipated backwards incompatible changes.

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@kostyafarber kostyafarber changed the title mount jupyterhighlight style manually Fix syntax highlighting for JSON viewer in Jupyter Notebook Nov 22, 2022
@jtpio jtpio added the bug label Nov 22, 2022
@jtpio jtpio added this to the 4.0.0 milestone Nov 22, 2022
@jtpio
Copy link
Member

jtpio commented Nov 22, 2022

Thanks @kostyafarber for working on this and tracking the issue down 👍

@krassowski
Copy link
Member

Is all this stylesheet does adding the green bold colour for keys and a few more colours for numbers/strings/null? Maybe we could just have a dedicated style in this extension rather than bringing another dependency and another copy of the stylesheet?

@fcollonval
Copy link
Member

Is all this stylesheet does adding the green bold colour for keys and a few more colours for numbers/strings/null?

FYI: I think it was done to get an identical styles for JSON in CodeMirror editor and in the viewer - that said I don't have cons to move to a new stylesheet.

Maybe we could just have a dedicated style in this extension rather than bringing another dependency and another copy of the stylesheet?

FYI: this is a new direct dependency. But it is not a new dependency as that package was created for and is needed by CodeMirror 6 (by the same author). Moreover it is carefully designed to not add stylesheet that are not necessary. And as in this case the theme is identical to the default Jupyter CodeMirror theme, it will not add a new stylesheet in JupyterLab page.

@fcollonval
Copy link
Member

I checked on Binder that indeed there is no additional stylesheet in JupyterLab.

@kostyafarber
Copy link
Contributor Author

Thanks @kostyafarber for working on this and tracking the issue down 👍

No worries! It was really @fcollonval that found the issue haha

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @kostyafarber

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

The examples CI failure is tracked in #13481

@jtpio jtpio merged commit ed04bc8 into jupyterlab:master Nov 24, 2022
@kostyafarber kostyafarber deleted the fix-missing-json-syntax-highlighting-notebook branch November 24, 2022 20:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants