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

removed outdated CSS #116

Closed
wants to merge 1 commit into from
Closed

removed outdated CSS #116

wants to merge 1 commit into from

Conversation

jorgenfinsveen
Copy link

@jorgenfinsveen jorgenfinsveen commented Jul 31, 2023

See discussion for issue #67

Closes #67

@jorgenfinsveen jorgenfinsveen linked an issue Jul 31, 2023 that may be closed by this pull request
@hgeorgsch
Copy link

How carefully have you tested this @jorgenfinsveen ?
I know that Sebastian had good reasons for the awkward !important flag; I'll see if I can find time to test it thoroughly, but I hope I do not have to.

@hgeorgsch
Copy link

I think you have to check very carefully if the Mathjax class can be created by the mathjax library, i.e. make a question using maths notation, render the page, and inspect the code surrounding the formula in the browser.

@jorgenfinsveen
Copy link
Author

@hgeorgsch I tested it by locating the elements which is part of the classes which is affected by the CSS block. I have documented my testing in a comment on issue #67.

I searched both the jazzquiz directory as well as the entire Moodle directory for any references to mathjax, but I could not find any

@sebastsg
Copy link
Member

sebastsg commented Aug 7, 2023

I think there were some changes in MathJax 3 that made the !important overrides not work anymore due to new rendering output. The default text align is center, and I remember it was distracting that the output wasn't right below the input box. My memory might be inaccurate, but if it does look like the old overrides could still be useful today, then I'd recommend figuring out a new way to do them.

This might be useful information: https://stackoverflow.com/questions/65264328/mathjax-3-change-css-styles

@hgeorgsch
Copy link

It is not sufficient to search the PHP code @jorgenfinsveen . I do not know what MathJax does; it does a lot. Hence we need to check the rendered HTML as well. Now, I did, so it is probably ok, but the MathJax_Display still appeared in CSS code, so I suspect I did not have a proper update of the code. I need to compare with the old version.

Copy link

@hgeorgsch hgeorgsch left a comment

Choose a reason for hiding this comment

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

Got it. Elements of class MathJax_Display are generated by MathJax for display math, that is TeX code in [ ... ] delimiters. That said, I do not see a difference between the two. I suppose the point is that this is redundant because MathJax no longer support the style def, as @sebastsg suggested. Or alternatively, the problem is that the enclosing tags have changed, so that they no longer match the target of the style.

Either way, the change does not make sense. Even if .MathJax_Display is no longer used, we do not want to move the style to .start-question-menu or .name. If that's the best we can do, we had better remove the style def altogether.

Now, I see that the style attempts to left justify the formula. This is a good idea because full screen jazzquiz makes a very wide box for the question, and the formula centred will often look bad. It would be a good idea if we could find a better way to achieve this.

If you do not have a better solution, I think you should comment out the original CSS code and make a new issue on the problem of centred equations. What do you think?

@hgeorgsch
Copy link

!important is removed on develop branch because it no longer works.
The .MathJax_Display class must be retained.
Rejecting pull request.

@hgeorgsch hgeorgsch closed this Aug 16, 2023
@sebastsg sebastsg deleted the bug/#67 branch April 20, 2024 08:14
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.

Grunt task complains about !important in styles.css
3 participants