Skip to content
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

#155 Allow overwriting of brand colors in flavours #346

Merged
merged 6 commits into from
Dec 29, 2024

Conversation

mwehr
Copy link
Collaborator

@mwehr mwehr commented Jul 18, 2023

Adds overwrite of brand colors for flavours.

@mwehr mwehr force-pushed the override_brand_colors branch from 18f4675 to 860471e Compare July 18, 2023 19:12
@mwehr mwehr changed the title #155 Allow overriding of brand colors in flavours #155 Allow overwriting of brand colors in flavours Jul 18, 2023
@mwehr mwehr force-pushed the override_brand_colors branch from 377ee23 to 832645f Compare July 20, 2023 09:08
@HsH-Wolf HsH-Wolf linked an issue Jan 15, 2024 that may be closed by this pull request
@lucaboesch lucaboesch force-pushed the master branch 2 times, most recently from d021aab to 4c0832b Compare March 1, 2024 10:32
@abias abias force-pushed the main branch 3 times, most recently from 5696e9a to b9fa4ca Compare October 21, 2024 15:33
@abias abias force-pushed the override_brand_colors branch from 832645f to 9e5a22d Compare December 28, 2024 07:29
@abias
Copy link
Member

abias commented Dec 28, 2024

Hi @mwehr ,

many thanks for this PR!
I admit that the review has been pending too long and I am really sorry about that. But I have now finally had the time to review it.

Generally, your solution approach is spot-on and the code is fine. You have correctly adopted the Moodle core concept to allow themes to manipulate the styles URL into Boost Union.

Anyway, I have made several adjustments to better align the code with the existing codebase and to further improve the feature.

In detail, I have done these changes in my "Review changes" commit:

  • Squashed all 4 commits into one and rebased the PR onto the latest main branch
  • Raised the theme version in version.php and fixed the upgrade step in db/upgrade.php
  • Better aligned flavours/css.php with /theme/styles.php (which was used as boilerplate obviously)
  • Fixed a bug with the parameters of the theme_styles_get_filename() function and re-added the routine to delete theme outdated sub revisions.
  • Renamed flavours/css.php to flavours/styles.php for more consistent naming.
  • Add a warning to the flavours settings page that the feature won't work when slasharguments is disabled (the slasharguments setting is planned to be removed in Moodle 5.0 by https://tracker.moodle.org/browse/MDL-83378, but until then we have to respect it).
  • Rename the global $flavourid variable and add some comments to it as I agree with the approach, but it still feels hacky.
  • Added a 'look_' prefix to the new database table fields
  • Moved the overriding of the background image in the SCSS stack process chain closer to the place where the background images are added.
  • Removed the MUC caching from the theme_boost_union_get_flavour_config_item_for_flavourid() function. You have used the flavours cache which already exists in Boost Union and which is a session cache (which means that it is user-dependent) but not an application cache (which you would need for storing the flavour settings). From my point of view, theme_boost_union_get_flavour_config_item_for_flavourid() does not need to cache anything in MUC as it is just run during the SCSS generation time. And if it should cache something, this would have to go into a dedicated application cache which should then be properly invalidated when the flavour settings change.
  • Add more headings to the flavour editing form
  • Add a "Pre SCSS" setting to the flavour as well
  • Removed the added colorpicker class as there was already an mform colorpicker element in Boost Union, it was just not located in the classes folder yet. I moved it into the classes folder and adopted the colorpicker validation rule (which is a really good idea) from your colorpicker into the existing colorpicker.
  • Allow the SCSS code validation to pass even if a CompilerException occurs. Otherwise, you would not be able to use SCSS variabled in the SCSS code. This is done similarly in the admin_setting_scsscode class.

In addition to the review of your PR, I went ahead and did the following:

I have pushed my changes to your PR branch and will now continue to write some Behat test which cover the new functionality.

Cheers,
Alex

@abias abias force-pushed the override_brand_colors branch from 9e5a22d to 0da081a Compare December 28, 2024 10:45
@abias abias force-pushed the override_brand_colors branch from e9f21ef to d0515fe Compare December 28, 2024 15:39
Copy link
Member

@abias abias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are red, but this is only because the PR contains the $theme->settings substring which is forbidden normally but correct in this page.

So the PR can be merged now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement: Allow overriding of brand colors in flavours
2 participants