-
Notifications
You must be signed in to change notification settings - Fork 344
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
Add LaTeX enabler and disabler to the Legay UI Component #8261
base: trunk
Are you sure you want to change the base?
Add LaTeX enabler and disabler to the Legay UI Component #8261
Conversation
Hi @fneumann, since this is in Draft-status, I assign you for the time being. Kind regards! |
Hi @klees, |
Hi @fneumann The entry in the DC is for features, as far as I know. For PRs it is, to my best knowledge, not necessary. We will review the public interface of your PRs and ask questions, request changes, if necessary. As soon as we believe this to have a form that is acceptable or if there arise questions that need JF involvement, we will set the JF label for the public interface to be discussed in the upcoming jf. What does this mean in this specific case; We will review the public interface parts of the suggestion an then set the JF label when all questions are resolved. 11.11 seems plausible, however this depends somewhat on the questions that arise. Does this help? |
Hi @Amstutz, yes, that helps. Thanks a lot. For a discussion at 11.11. it would be very helpful to know your interface relates questions, so that I can see if there are any obstacles regarding the overall concept: a) The ILIAS core should be unambiguous regarding the places where MathJax is rendered and how this is done. The plugins is crucial for the acceptance of the PR by the SIG Mathe+ILIAS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fneumann,
Thanks a lot for your contribution to the UI components!
Please note, this review will only focus on the public interface, so we can pass this through JF first.
About the concrete changes, please answer the following questions:
- Local disabling/enabling: can you explain why we need to disable or enable the latex processing on
LatexAwareComponent
's locally? Our goal is to mark semantically strong components in the future, which will always render latex, if any is provided. It seems weird to mark a component, but still allow to suppress the syntax. From an UI framework perspective, this will only be a temporary measure anyways, so if this is not required at the moment, we would like to drop this functionality. -
mathjax
npm package: can you explain how the disabling and enabling of the client-side latex processing with themathjax
package works exactly? E.g. if a parent element is disabled, are enabled child elements processed or skipped? Is there any difference between CSS classes or HTML tags when disabling/enabling the processing (I read this is possible)?
Please implement the following changes:
- Global disabling/enabling: the paper on text-handling in ILIAS proposes a concept to talk about different "shapes" of text, such as latex, which are defined classes like e.g.
TextWithLatex
would be. Classes like this can then be used to set certain expectations for specific areas of ILIAS. So, once we have implemented (stages) of this concept, enabling or disabling latex would undermine these expectations and the underlying concept. I therefore ask you to remove the feature-switch altogether, and make latex permanently available. Since we are addingmathjax
as a dependency, this should also not be an issue. -
MathJaxConfig
: the current architecture and mechanics of the UI framework already allow you to perform everything you are trying to achieve using theMathJaxConfig
interface and implementation. Plugins can easily manipulate these things by exchanging the defaultUI\Renderer
, so from an UI framework perspective, this configuration isn't necessary. The only thing that cannot be addressed from the UI framework's side, is providing required assets to the ILIAS content page exporter; but this is also true for every other component, I assume. Therefore, please get rid of this configuration and implement it directly in the JavaScript configuration (which can also be replaced by plugins) or the renderer. - Latex delimiter: one thing missing from the
MathJaxConfig
is probably the delimiter used for client-side latex processing. Please implement anUI\Component\LatexDelimiter
interface for this, which returns one or more pair of delimiters in somegetLatexDelimiters(): array
method. You can inject this into the UI framework with a default implementation as an anonymous class. - Rendering process: we do not want to introduce renderers, which are different to the
ComponentRenderer
interface. Please implement the latex rendering process with our established patterns. You can orient by looking at e.g.Tooltip
s or contact us, if we should discuss this in person.- Inject the
LatexDelimiter
configuration intoAbstractComponentRenderer
and all implementors. - Implement the latex rendering process inside the
AbstractComponentRenderer
. - Call the latex rendering process from the component renderer(s) of
LatexAware*
components.
- Inject the
-
LatexAwareComponent
: if local disabling/enabling is really required, please simplify the interface by combining the setters (withDisabledLatex(bool $is_disabled)
) and removing the getters, as they are an implementation detail. -
LatexAware*
terminology: we already have an established pattern for naming things like this. Please rename allLatexAware*
related interfaces and classes to matchHasLatexContent*
. -
mathjax.md
: this document contains information which is mostly unrelated to the UI framework. Please move this document to the more general./docs/
directory.
Thanks for your comprehensive review of the PR. Before I start refactoring the code, I would like to give you explanations for each topic of your review. I hope you find the time to read and answer them. Local disabling/enablingThe interface LatexAwareComponent is proposed for the Legacy component. All places where Latex is currently processed are rendered as Legacy components. These are partially complex contents e.g. in the PageObjectGUI, and I expect it taking several ILIAS releases before they are supported with dedicated UI Components. On the other hand, there are far more usages of Legacy where latex should not be processed. To make a distinction, we either must introduce a second Legacy component or add a Latex enabling switch to the component. The local latex disabling is intended for situations where the outer component (e.g. a CoPage, see above) supports latex, but some parts of it should not process it. MathJax npm packageIf the MathJax script is added to the page, it will normally process the whole page content. This can be restricted by adding CSS classes to elements of the content. The strategy is to add a Additionally, MathJax disables latex processing generally for certain HTML elements where its processing would break HTML rules (e.g. in options of select). These elements can be configured, too, but the proposed script keeps the default. Global disabling/enabling: MathJax is not a small script. It is a large bundle of modules that are dynamically loaded to the page once the MathJaxConfigAs mentioned for global disabling/ enabling, we need the Latex delimiterI don’t think that is needed. The Latex delimiters are directly defined in the Rendering ProcessRegarding the Tooltip example: do you mean the LatexAwareComponentPlease note that with nested components we may need a three-state logic: enabled/disabled/inherit. We can implement the inheriting as a default without an explicit setting. I would then prefer a positive function withLatexEnabled (bool $enabled). This is what will be used by other ILIAS components dealing with Latex. LatexAware*No objection. Mathjax.mdI Agree. Best regards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fneumann,
Thx for your explanation. I stumbled upon your sentence,
The local latex disabling is intended for situations where the outer component (e.g., a CoPage) supports latex, but some parts should not process it.
which suggests a pattern we should avoid. This puts the decision of disabling/enabling latex onto our consumers, forcing them to have knowledge about their context. This weakens the composability of our components and also complicates the introduction of new components and the maintenance of all existing components, since they need to react to possible nesting (context).
Instead, we should focus on accepting specific text shapes, such as TextWithLatex
, which the component can handle internally. These shapes should be rendered within simple HTML elements that don’t support the nesting of complex HTML structures. These elements alone should then receive the latex processing class. This will avoid problems where components or consumers need to think about their context.
However, since consumers of the UI framework most likely do not yet store latex in a way that can communicate these shapes, I suggest to introduce a Legacy\LatexContent
component for the time being - without any disabling or enabling. This will also indicate a pattern we do not want to pursue. You probably need to wait until #8436 is merged, because this PR extends the legacy component family so new ones can be introduced.
So in summary, we do insist on all of our requested changes, except the delimiter; if this configuration becomes necessary, we can still implement this to a later point.
This PR proposes to add a new interface LatexAwareComponent to the Legacy UI component.
The new inteface add the functions
withLatexEnabled
andwithLatexDisabled
that control where LaTeX exprssions in the HTML content of the component should be rendered by an activated MathJax script on the page. See the example latex.php for details on how they are used.A new interface LatexAwareRenderer is proposed for the renderer of the Legacy component.
Its function
withMathJaxConfig
sets a data object of the interface MathJaxConfig. The functionregisterMathJaxResources
registers the resources needed for MathJax. Additionally the functionaddLatexEnabling
andaddLatexDisabling
enable or disable the rendering LaTeX within the component.Both interfaces work together in the
render
function of the component:The implementation of these interfaces is supported by the traits LatexAwareComponentTrait and LatexAwareRendererTrait. The enabling and disabling is done by wrapping the content in an element with a class that is recognized by MathJax. The class is taken from the MathJaxConfig.
The LatexAwareRenderer is also used by the Page Renderer. It checks for
isMathJaxEnabled
andgetGlobalDisablingClass
to set a class in the body of the page that prevents a rendering of MathJax where LaTeX is not enabled.Currently the new interfaces and traits are only used for the legacy component. They may be used by dedicated components in the future.
This PR is an excerpt of a larger PR that also includes the setup od MathJax and the usage in ILIAS components:
Streamline LaTeX usage
It is related to the coresponding feature request.