remove mathjax form main xmodule requirement and add it on problem addin... #6048
Conversation
zubair-arbi
force-pushed
the
zub/bugfix/tnl-823-remove-mathjax-from-main-dependency
branch
from
ef3d181
to
6642c4d
Compare
|
unfortunately, we still need mathjax on every unit in studio. Is there a way to load it, but not as a dependency? |
|
@adampalay I also added mathjax in studio_xblock_wrapper so now mathjax will also work for html or other components which don't use formula_equation_preview.js. |
|
You can test on sandbox here http://studio.zubair-arbi.m.sandbox.edx.org |
|
@andy-armstrong can you please review and check if the change is reasonable? |
|
It feels faster to load a unit now! Question: if mathjax is slow to load, or doesn't load, will that mean that xblocks will be slow to load--or blocked from loading--in studio? |
| @@ -169,4 +169,11 @@ formulaEquationPreview.enable = function () { | |||
| $('.formulaequationinput input').each(setupInput); | |||
| }; | |||
|
|
|||
| formulaEquationPreview.enable(); | |||
| try { | |||
There was a problem hiding this comment.
We aren't using requireJS in the LMS currently (although we're edging towards using it). I'm nervous that this isn't going to work there. Have you tested this in the LMS?
@cahrens Are you familiar with how this works?
There was a problem hiding this comment.
Is this change even necessary given the studio wrapper change? Or is it still needed so that this code doesn't run before the wrapper async process completes? Does there need to be similar logic in every usage of MathJax?
There was a problem hiding this comment.
@andy-armstrong You are right about that requireJS is not being used in LMS that's why I added try-catch block. Since display method inside updatePage method uses mathjax that's why I added a separate dependency here to be on safe side (like if mathjax not loaded first time with xblock_wrapper for some reason then loading this file will try again to load mathjax) so this change is not necessary.
There was a problem hiding this comment.
I don't think that whether or not require exists is a valid way of telling CMS vs. LMS. If you expect mathjax to be on the window object, check if it is (and when RequireJS is added to LMS, make sure that the config says that mathjax exports the right thing).
|
The sandbox isn't working for me. It never even gets to the login page. |
| <script type="text/javascript"> | ||
| $( document ).ready(function() { | ||
| require(['jquery', 'mathjax'], function() { | ||
| window.MathJax = MathJax; |
There was a problem hiding this comment.
Is there any dependency between this code and the change below (formula_equation_preview.js)? Does window.MathJax need to be defined before formulaEquationPreview.enable() is executed?
There was a problem hiding this comment.
@cahrens since mathjax could be needed on every unit page e.g, a user puts some math content "(\frac{3}{4})" in html component which don't requires ormula_equation_preview.js so I added mathjax dependency here since xblock wrapper is added with each xblock. Also since this is a soft dependency so if mathjax won't load for some reason then page will render just fine.
For formula_equation_preview.js formulaEquationPreview.enable() requires mathjax (hard dependency) so I added separate requirement for that method (if mathjax is not loaded with xblock_wrapper then it will try again).
There was a problem hiding this comment.
Why the dependency on JQuery?
If we go this approach, a big comment is needed here explaining what is going on.
What does the user see if MathJax is needed? Since this is in document ready, will there be an additional delay for the text is rendered (does it render in a different font and then re-render)?
There was a problem hiding this comment.
You are right I shouldn't add dependency on JQuery.
Since mathjax will load after the page is rendered (with normal font) so after mathjax is loaded it will re-render (with mathjax font).
|
@andy-armstrong I have rechecked sanbox is working http://zubair-arbi.m.sandbox.edx.org/ |
|
@cahrens @andy-armstrong does these changes look reasonable? |
| formulaEquationPreview.enable(); | ||
| try { | ||
| require(['jquery', 'mathjax'], function() { | ||
| formulaEquationPreview.enable(); |
There was a problem hiding this comment.
I'm still nervous if this will work properly when part of the LMS is using requireJS (per the PR that is about to merge). We need to make sure that the RequireJS config there exports mathjax on the window object. I'd prefer to hold on this PR until #6158 merges, and then update the require config if necessary.
|
Another thing to keep in mind-- xmodules exist in other places in Studio besides the unit/container page. For example, static pages and course info (I'm not 100% positive that course info uses xmodules, but static pages certainly does). So we would need to make sure that the right thing happens in those locations as well. 👎 I think more thought is needed on this change. |
|
@cahrens I have checked the static pages and course info (updates and handouts) don't use mathjax. You can check by adding some math expression like "(\frac{3}{4})". |
|
Note that the first PR introducing RequireJS is now in master-- https://github.com/edx/edx-platform/pull/5829. |
zubair-arbi
deleted the
zub/bugfix/tnl-823-remove-mathjax-from-main-dependency
branch
...g
TNL-823
@waheedahmed @mlkwaqas @adampalay