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

landing page: support different MathJax delimeters #2794

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

anikachurilova
Copy link
Contributor

Add support for $ and \ MathJax delimeters.

Examples of records that are using different types:
https://repository.cern/records/fp18d-jc149
https://zenodo.org/records/8292839

@@ -407,6 +407,14 @@ <h4>{{ _("Request access") }}</h4>

{%- block javascript %}
{% if config.THEME_MATHJAX_CDN %}
<script type="text/javascript">
Copy link
Contributor

Choose a reason for hiding this comment

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

move the entire block to invenio-theme, but pay attention to the else below include config.THEME_JAVASCRIPT_TEMPLATE

@anikachurilova anikachurilova force-pushed the LaTeX-rendering branch 2 times, most recently from 7334543 to 3b49d2a Compare August 28, 2024 11:46
Comment on lines 37 to 39
{%- block javascript %}
{{ super() }}
{%- endblock javascript %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: If we simply call super we can simply remove the block and it would be the same, no?

Suggested change
{%- block javascript %}
{{ super() }}
{%- endblock javascript %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that in the child component I'm calling super.super(), some tests were failing because it couldn't find javascript block in here.

Comment on lines 9 to 20
{% if config.THEME_MATHJAX_CDN %}
<script type="text/javascript">
window.MathJax = {
tex: {
inlineMath: [['$', '$'], ['\\(', '\\)']],
processEscapes: true // Allows escaping $ signs if needed
}
};
</script>
<script type="text/javascript" src="{{ config.THEME_MATHJAX_CDN }}"></script>
{% endif %}

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I think this is duplicated, if we are adding it to the base page.html (as done in this PR) it shouldn't be needed here? unless I am missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jrcastro2 jrcastro2 Aug 29, 2024

Choose a reason for hiding this comment

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

Hmmm I would expect that if it's already added in invenio_theme/templates/semantic-ui/invenio_theme/page.html it's present in all the views as it's the global template we use, unless I am wrong.

I am also suprised about the super.super(), I think that this is skipping the parent super and going to the grandfather, see this for reference: https://jinja.palletsprojects.com/en/3.1.x/templates/#nesting-extends

I find it a bit confusing to follow this and would expect to have a simpler way to enable it.

I think that adding it to the base global page is correct and from there I would simply call super() if we ever need to override that block. Maybe I am missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to review all this IRL, because it is not clear to me why we need it. From a quick look, we always use THEME_JAVASCRIPT_TEMPLATE.

Copy link
Contributor

@jrcastro2 jrcastro2 left a comment

Choose a reason for hiding this comment

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

Reviewed with @sakshamarora1 LGTU! 🚀

@kpsherva kpsherva merged commit 96cee9d into inveniosoftware:master Sep 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LaTeX rendering
5 participants