UI/Metabar: engage/disengage more-button

[nhaagen]

https://mantis.ilias.de/view.php?id=37151

I merely converted the late slate.js and metabar.js into ES6; I left out mainbar.js for now and economic reasons and split up the dist-files; also, the jquery-parts are still in ;(

[klees]

@thibsy @Amstutz: This looks good to me. Please have a look as well. Please notice @nhaagen|s first comment =)

[thibsy]

Hi @nhaagen

Thanks a bunch for turning the main-controls and slates into ES6 modules. This is already a huge improvement of readability and maintainability :).

About the concrete changes, please answer the following questions:

• A11Y: you have mentioned a mantis ticket describing an accessibility issue. How is this PR related to the ticket? If I understand the report correctly, the issue still remains because the slates do not provide an aria-expanded attribute. Am I missing something?

Please consider the following suggestions:

• Bundle structure: we have not fully settled on how we want to manage our bundles yet, so this point is highly optional, but IMO we should try to only use the dist/ folder to provide different formats of a bundles business logic, e.g. standard, minified, or ES6 in the future. This PR introduces a separate file to load additional functionality. Therefore, I recommend to merge maincontrols.js and mainbar.js into one index file.
• Filenames: according to Airbnb's naming conventions, the filename should exactly match its default export. This is not the case for all newly added files, except maincontrols.js.

Please implement the following changes:

• Encapsulation: there are many public methods and properties of Metabar and Slate which should be made private or internal (not accessible by this) instead. If a property must be accessible, please do so with an according getter/setter.
• cls* properties: these properties should be internal, while at it, you could also rename them to something like class* or classFor* or similar, since cls is not a known acronym.
• il: you are accessing other components globally via il on several occasions. Please try to inject the according functionality in the constructor already and pass it down from the index file. Let me know if this is suboptimal or not possible for some usages.
• maincontrols.js:22: you initialize an unused property/namespace here (il.UI.table), please remove it.

Kind regards,
@thibsy

[nhaagen]

Hi @thibsy ,
thanks for looking at this.

A11Y: you have mentioned a mantis ticket describing an accessibility issue. How is this PR related to the ticket?

The actual changes of this PR can hardly be seen; the linked ticket asks for the metabar more-button to carry an
aria-expanded. Here is the code actually preventing that:
https://github.com/ILIAS-eLearning/ILIAS/blob/857cb72fa34596312ec819a841bd52e7c195b5f1/src/UI/templates/js/MainControls/metabar.js#L70C1-L72C5

With deleting the line and checks in place, the refactoring was made necessary. Metabar depends on slate, so both had to be touched ;(

Bundle structure:
Filenames:

I fully agree. In the end, there should be but one file "maincrontrols.min.js" containing slate, metabar and mainbar.
However, all of these are quite central to the UI, and Mainbar is huge. All of them, mainbar, metabar and slate, need to be rewritten eventually, but that's a thing of its own. For now (and in this PR), I'm not touching mainbar.

Encapsulation
il: you are accessing other components globally via il

I set quite a few methods and props to private and tried my best to inject dependencies; it's still quite entangled, though, but visible a least.

[thibsy]

Hi @nhaagen

Thanks for the quick feedback and for implementing the changes.

About the concrete changes, please answer the following questions:

• A11Y: thanks for pointing out the exact change, I missed that :). However, I think the issue is still not fully fixed by this PR alone, because according to the linked report of Materna, sub-menus also need to provide an aria-expanded attribute. If I currently click e.g. the user sub-menu in mobile view, this attribute is not set. I've opened another ticket for this, since the current targets the "more" button.

Please implement the following changes:

• class* properties: I think these properties need to be internal constants, instead of private properties, since their values do not change. This may lead to some methods needing to be internal as well, because they will not use this anymore. Please do the same for Slate.#replacementType.

Thanks for bearing with me!

Kind regards,
@thibsy

[thibsy]

Picked for release_8

• fc218e3