ILIAS
ILIAS copied to clipboard
ILIAS-eLearning
fix js error on initial toast load
https://mantis.ilias.de/view.php?id=34590
This PR mostly reverts changes made in https://github.com/ILIAS-eLearning/ILIAS/pull/5012 and add all toasts that are preexisting to the toast container withing the pagebuilder to prevent an additional request.
@lscharmer @Amstutz @klees Pls leave your opinion on this. I wouldnt see the need to take that to a JF until the is no major discussion point. Feel free to open such.
Greeting, @iszmais
Hi @alex40724,
you are listed as implicit maintainer for Services/Notification, hence I assigned you. Please reassign if there is a person you deem more suitable.
Thanks!
It is Services/Notifications, so I deassigned @alex40724 and assigned @mjansenDatabay
The changes LGTM in general.
@chfsx There is one change in src/GlobalScreen (https://github.com/ILIAS-eLearning/ILIAS/pull/5121/files#diff-5769142c345fc3a6bc2301605fe73f7876384f4504e108611a562c99fff70f2b). Do you have any objections?
@chfsx Is there still something missing your requirements? If yes feel free to leave a comment or otherwise i would appreciate your approval.
Greetings, @iszmais
Hi @iszmais
Yes actually my comment from last time is still relevant, but I that you have pushed something to this. We should look at this together. The StandardPageBuilder should not get too much dependencies, but especially no new ones. You have addressed this by allowing the PagePartProvider to supply the TostContainer. But also in StandardPagePartProvider we should keep the dependencies small. For certain things this is not possible (e.g. breadcrumb with ilLocator). In the case of the toasts it would be a good idea to collect the single toasts by ToastProviders and a ToastCollector. Or you can use (maybe better) the existing NotificationCollector, like it is done for the AdministrativeNotifications (see for example \ILIAS\GlobalScreen\Scope\Layout\Provider\PagePart\StandardPagePartProvider::getSystemInfos). Otherwise the toasts will have some kind of special status and it will be difficult to keep track of them in the future.
would you like to have a chat on discord next week?
Hello @chfsx ,
I abstracted the toast provider into an own scope within the global screen. Imo it should not be combined or integrated inside the already exsiting notification scope since the intention of creating the toast was do separete the over the years tangled chat and notfication therefore i think we should start that tangeling again.
Maybe you already can take a look into this i will also fix the tests to it. I would prefer to present the changes of the existing providers (ChatInvitation, Badge, WhoIsOnline, ...) in seperate PRs wich are depending on this just to make it easier for the target maintainers.
Greetings, @iszmais
Hi @iszmais
Thank you for your comments and the discussion in https://github.com/iszmais/ILIAS/pull/7. I pushed you two more commits there that we should still include (one is not directly related to your change, but I noticed it because it didn't have any data on the ToastProviders in the artefact yet). Your explanation of the distinction between toasts and notifications convinced me, so we can merge it like that!