Moodle 404 stable #76
Conversation
| /* End ALLY-11 fixes */ | ||
| /* =====================================================*/ | ||
|
|
||
|
|
There was a problem hiding this comment.
@Toni-D1 What do you think about having fixes like this !important hardcoded in this repository? They could be applied in our integration css instead. Alternatively, we could put all Moodle styling only into this plugin to avoid overrides.
There was a problem hiding this comment.
Hmm.
It makes sense to me that overrides to ally styles should be in the ally repo, but I wouldn't want that to affect the workflow/difficulty for the plugin work.
But you are right, the liberal use of !important means we are setting ourselves up for css issues. Let's have a chat, maybe there is a middle ground that tidies this up without creating workflow issues.
There was a problem hiding this comment.
Just for reference, we have our Moodle specific styling under src/integration/moodlerooms/moodlerooms.css (ally-ui).
There was a problem hiding this comment.
Yeah - I saw. It's not much, but it still seems to reference ally components... It's a bit of a mess for sure :(
There was a problem hiding this comment.
@Toni-D1, @dominik-gmiterko - I would recommend that all moodle styles are implemented in the moodle ally filter plugin, not the integration css, so that we don't have to use !important. It does not make sense to put them in moodlerooms.css since it will not account for multiple versions of moodle. The ally filter plugin has branches for each version of moodle and the css in each branch will be different to account for the markup of that moodle version. If you would like to style things for moodle in moodlerooms.css you will need a mechanism to account for moodle versions - a body class with the version in would make sense. Sadly moodle doesn't do this by default, but I could write some JS in the filter to add the version to the body class - e.g. "MOODLE_404" you could then target different versions of Moodle for the appropriate CSS.
There was a problem hiding this comment.
Following on from my previous comment, I would still recommend that the Moodle filter is used for styling, otherwise you will have to re-test with all previous versions in use by customers and then ensure customers install a new version of the filter plugin that adds in the body class for you to target versions in moodlerooms.css.
There was a problem hiding this comment.
Agree, as we cannot at this time distinguish version of Moodle from our side and we don't want to break styling for previous versions we cannot resolve this easily. So for now l would leave it as it is. We can consider ways to get this more streamlined in the future. PR - approved from my side.
Upgrade to Moodle 4.4 (2024042203), update tests