Skip to content

Restore syntax highlighting for mimetypes with more than one identifier #15175

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.

Sign up for GitHub

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

Jump to bottom
Merged
fcollonval merged 6 commits into from
Oct 2, 2023
Merged

Restore syntax highlighting for mimetypes with more than one identifier #15175

fcollonval merged 6 commits into from
Oct 2, 2023

Conversation

@jans-code
Copy link
Contributor

@jans-code jans-code commented Sep 28, 2023 •
edited by fcollonval
Loading

References

Fixes #15172

Code changes

Checking if 'spec.mime' is an array, and if it is using the first string for 'widget.content.model.mimeType', else business as usual.

User-facing changes

n/a

Backwards-incompatible changes

n/a

Edit to link the issue.

@jupyterlab-probot
Copy link

jupyterlab-probot bot commented Sep 28, 2023

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

@welcome
Copy link

welcome bot commented Sep 28, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@krassowski krassowski added the bug label Sep 28, 2023
@krassowski krassowski added this to the 4.0.x milestone Sep 28, 2023
krassowski
krassowski reviewed Sep 29, 2023
View reviewed changes
krassowski
krassowski reviewed Sep 29, 2023
View reviewed changes
fcollonval
fcollonval requested changes Sep 29, 2023
View reviewed changes
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 @jans-code

I have a minor improvement but otherwise the logic is good.

packages/fileeditor-extension/src/commands.ts Outdated
Comment on lines 571 to 575
if (spec.mime.length == 0) {
widget.content.model.mimeType = 'text/plain';
} else {
widget.content.model.mimeType = spec.mime[0] as string;
}
Copy link
Member

@fcollonval fcollonval Sep 29, 2023

Choose a reason for hiding this comment

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

We can be more condense for the fallback. And there is a variable to avoid hard coding text/plain.

Suggested change
if (spec.mime.length == 0) {
widget.content.model.mimeType = 'text/plain';
} else {
widget.content.model.mimeType = spec.mime[0] as string;
}
widget.content.model.mimeType = spec.mime[0] as string ?? IEditorMimeTypeService.defaultMimeType;

Copy link
Contributor Author

@jans-code jans-code Sep 29, 2023

Choose a reason for hiding this comment

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

Much nicer indeed!

@fcollonval fcollonval mentioned this pull request Sep 29, 2023
Merged
krassowski
krassowski approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @jans-code!

@krassowski krassowski changed the title fix: syntax highlighting for mimetypes which have more than one id Restore syntax highlighting for mimetypes with more than one identifier Sep 29, 2023
fcollonval
fcollonval approved these changes Oct 2, 2023
View reviewed changes
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 @jans-code

@fcollonval fcollonval merged commit 70b1e47 into jupyterlab:main Oct 2, 2023
@welcome
Copy link

welcome bot commented Oct 2, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@fcollonval
Copy link
Member

fcollonval commented Oct 2, 2023

@meeseeksdev please backport to 4.0.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Oct 2, 2023
@jans-code @meeseeksmachine
Backport PR jupyterlab#15175: Restore syntax highlighting for mimetyp…
b013f18 
…es with more than one identifier
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 2, 2023
Merged
krassowski pushed a commit that referenced this pull request Oct 2, 2023
@meeseeksmachine @jans-code
Backport PR #15175: Restore syntax highlighting for mimetypes with mo…
efde1b3 
…re than one identifier (#15205)

Co-authored-by: Jan Arman Parpin <112409139+jans-code@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@krassowski krassowski krassowski approved these changes

@fcollonval fcollonval fcollonval approved these changes

Assignees

@jans-code jans-code

Labels

bug pkg:fileeditor

Projects

None yet

Milestone

4.0.x

Development

Successfully merging this pull request may close these issues.

.ipynb files have no syntax highlighting in texteditor

3 participants

@jans-code @fcollonval @krassowski