-
Notifications
You must be signed in to change notification settings - Fork 396
Default-HTML-Frame for Mail in System Style (1/3) #9564
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
Conversation
c230e6a to
3fe04a4
Compare
|
Hi all, We apologise for the delay here. We want to get this included in ILIAS 11, so we try to get the full review (interface and implementation) done until Monday next week, so we can also present this at the last JF before release. Please tell us if you already know you will not be able to implement requested changes until the end of next week, then we will prioritise other PRs. |
|
Hi @thibsy, thanks a lot for your efforts. As far as I know, the deadline is Tuesday, 28 October 2025. Best regards, |
klees
left a comment
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 @kjansen2608,
Thanks a lot for your contribution to the UI components!
Please note that I will try to abbreviate FQDN's and refer to the public interface as C and implementation as I
About the concrete changes, please answer the following questions:
- HTML and DOCTYPE tags: since this template is now part of the UI framework, it would probably be good for us to know, what these definitions do exactly. Can you explain, why i.e. a simple
<!DOCTYPE html>is not enough, and why both of these tags need special attributes?
Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:
-
C\Layout\Page::withFooterURL: you are more familiar with the mail use-cases than I am, so I leave this up to you: I think we leave many choices up to the consumer if we accept a link UI component here. Wouldn't it i.e. be better if either all or none of the footer URLs open in a new viewport? For consistency I suggest to accept anILIAS\Data\Linklink here, so we can keep this universal and an implementation detail. - Logo-source: you convert the image into a base-64 string in your example. I assumed you can also provide an actual file path, is this not the case? Could you elaborate why this would be different to the other cases like the stylesheet, where you accept a path? If there are no good reasons for the difference, it would be preferable to use a path for both.
Please implement the following changes:
- Correctness on construction: we follow this principle most of the time. The
C\Page\Standardwas designed with many mutators, because of its orchestration viaILIAS\GlobalScreen. I don't think we need this kind of flexibility and complexity with theC\Page\Mailcomponent. Creating a mail, IMO, should not happen across many ILIAS components or layers. Please make all (mandatory, IMO all except footer-URL?) arguments inC\Page\Factory::mail()non-nullable, keep the others as is but order them correctly. Strip down theC\Page\Mailinterface completely, remove all mutators andhas*methods entirely (interface and implementation), only keep getters inside the implementation. - Stylesheet validity: please throw an
\InvalidArgumentExceptionduring construction already if the file cannot be found, instead of in the renderer. -
ilUIFrameworkusage: this class does not exist anymore. Please check why this was necessary and remove it, if there is no need anymore. -
I\Page\Renderer::render(): please check theinstanceofthe implementation, not the interface. You can read why this is important here -
tpl.mailpage.htmlI: you declare thexmlnsattribute on the<html>tag twice, I assume this is not intentional. Please remove it. -
tpl.mailpage.htmlII: you declare the<head>tag twice, I assume this is not intentional. Please remove it. -
I\Page\Mail: please make theprivatepropertiesprotectedto leave them open for extension.
|
☝️ I have posted this, but @thibsy did the most of the work. Thanks! |
|
|
||
| namespace ILIAS\UI\Implementation\Component\Layout\Page; | ||
|
|
||
| use http\Exception\RuntimeException; |
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.
This looks wrong here.
3fe04a4 to
cc7c023
Compare
|
thank you for your review.
Yes HTML 5 would be a better fit - we just took it from the old template. Due to limited time before the release, we won't be able to test it beforehand with all the email clients we listed above in the PR description. But we don't expect any issues with it. We’d be happy to include it in our roadmap and perform the testing afterwards.
done
We cannot use a file path because they are not supported in every email client. Even the stylesheet is not using a file path it is included with
done
done
done. This was a mistake because of a rebase
done
done
done
done Kind regards, |
thibsy
left a comment
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 @kjansen2608,
Thx for implementing the changes so quickly.
I have found some nit picks which I have commented inline. In addition, please:
- Logo-src: we understand that assets cannot be included, but it makes the lives of our consumers much easier if they can provide asset paths and we will do the work. Therefore, please implement the "logo src" similar to the stylesheet (validation of path inside constructor, conversion in renderer) and rephrase the according variable-, property-, and method-names to match "logo path" as well. If consumers are expected to provide specific mime-type(s), i.e. for compatibility with
base64_encode(), also add a usage rule for this to the factory description.
Afterwards we can merge this.
Thx and kind regards,
@thibsy
components/ILIAS/UI/src/Implementation/Component/Layout/Page/Renderer.php
Outdated
Show resolved
Hide resolved
components/ILIAS/UI/src/Implementation/Component/Layout/Page/Renderer.php
Outdated
Show resolved
Hide resolved
components/ILIAS/UI/src/Implementation/Component/Layout/Page/Renderer.php
Outdated
Show resolved
Hide resolved
cc7c023 to
8fcb5ce
Compare
|
Hi @thibsy,
sorry for the confusion, this is already possible (and intended) with the current solution, only the examples use a base64 data URL, in the mail implementation PR #9566 this is implemented with a path to an attached image (cid). One just needs to be careful as the stylesheet path must be embedded, so this expects a file path pointing to a file on the server but the logo does not need to be embedded, so it accepts a file path pointing to the attachments or a data URL. Best regards |
8fcb5ce to
7d92abf
Compare
|
@lscharmer, I will paraphrase to make sure we understand this correctly: you intentionally leave the possibility to embed a logo from a source which is NOT on the ILIAS server. This must mean, you have a use-case for this in mind? From our POV this currently makes not much sense, maybe we can talk about this briefly on Discord to save time? |
adds mail component adjust docu and move css adds delos.css to mail template display logo inline changes html line-breaks adds Test for MailPage uses default header-icon instead of new .jpg fix test removes URL from template updates mail-docu and renames tpl-var fix header css design fixes Fix CS for Mail-Interface Fix CS for Mail Implementation remove unrelated fixes Fix CS for Mail test Fix CS for Renderer Fix CS change ui-mail content type from string to Content use mail.css instead off delos.css build delos.css fix tests fix docu use Dataprovider remove testGetContent removes usage of call_user_func and add commas adjust docu improve mail renderer apply all suggestions fix tests removes mail.css
7d92abf to
e895000
Compare
|
Hi @thibsy,
Best regards and thanks again for your time :) |
thibsy
left a comment
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.
Thx @lscharmer for your explanation. Changes LGTM now!
|
Jour Fixe, 10 NOV 2025: Yvonne notified us about changes in the public interface through this PR. Thanks a lot for this very valuable improvement. Changes were already committed to trunk before release 11 branch has been created and are therefore available in ILIAS 11, too. |
What has been changed?
ILIAS system mails previously used a basic HTML template that included the default ILIAS logo and the installation title "Open Source eLearning".
Customizing this mail layout required manually overwriting the corresponding template file in the System-Style, which was technically functional but not aligned with system-wide design principles.
We developed a solution that:
Why was it implemented this way?
The main goal was to centralize styling logic and make mail templates easier to manage when applying individual System-Stylings. By providing a new SCSS file which can be customized via System-Styles , we aimed to:
This decision was based on discussions we had with @Amstutz, where he gave us the initial impulse to bring the mail layout into the UI framework.
A technical challenge we encountered was embedding the logo.
To ensure that the logo would be delivered as an attachment and correctly referenced in all email clients, we had to directly embed the image from a given file path.
This method gives full control over how the image is embedded and allows the mail to include it as an actual attachment, increasing compatibility and flexibility for users.
How was it tested?
Manual testing was conducted across a wide range of mail clients to ensure rendering consistency.
This was particularly important because many email clients use outdated rendering engines, leading to inconsistent support for modern HTML and CSS standards.
Not all clients could be tested due to device constraints.
Results:
Notes for reviewers
We're still looking for a clean and user-friendly way to notify developers and designers of System-Styles that two SCSS files need to be compiled now instead of just one.
Currently, this introduces a second compilation layer, which is not ideal.
An alternative approach, extracting the mail section from the compiled CSS, was discussed in the same meeting with @Amstutz, but was ultimately dismissed by mutual agreement, as it was considered less sustainable in the long term.
We're considering developing a small script to assist with this step. Feedback on this point is welcome :)
Related Tickets / References
Feature Request:
https://docu.ilias.de/go/wiki/wpage_7280_1357
Due to the work on the UI component and the PR, the FeatureRequest will be reworked accordingly before it is presented again to the JourFixe for 11/Trunk.
Internal Reviews:
Related PR's: