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

Improvement: Do not force child themes to reimplement any theme settings, but allow it #249

Open
danowar2k opened this issue Mar 17, 2023 · 1 comment · May be fixed by #250
Open

Improvement: Do not force child themes to reimplement any theme settings, but allow it #249

danowar2k opened this issue Mar 17, 2023 · 1 comment · May be fixed by #250
Assignees
Labels
child theme support Something which helps the creation and management of Boost Union child themes improvement Something which improves an existing feature in some way (UX, UI, Design, Functionality)

Comments

@danowar2k
Copy link
Collaborator

Right now, Boost Union implements a mix of forcing child themes to reimplement some settings and, in case of other settings, explicitly takes Boost Union's settings.

The first case could lead to unnecessary duplication of settings code.
The second case could maybe lead to duplication of function code if one would prefer to implement a theme's settings as "all settings for theme appearance should be configurable in a theme's settings pages).

I propose a function that returns a child theme's setting value if that setting exists in that child theme and otherwise returns Boost Union's setting value.

I'll prepare a pull request with that function (probably in lib.php or locallib.php, even if I like autoloaded classes wrapping this stuff, i.e. "class settingsutils"), and if the idea is accepted, I'd complete the PR with a bigger commit replacing all settings use with using this function.

danowar2k added a commit to danowar2k/moodle-theme_boost_union that referenced this issue Mar 17, 2023
…ow it

Added a method that allows child themes to duplicate boost union settings, but doesn't force them to do so, in case child themes want to just reuse boost union's configuration
@abias abias added the feature Something which is a new feature or big improvement label Mar 18, 2023
@abias abias added the child theme support Something which helps the creation and management of Boost Union child themes label Mar 21, 2023
@wiebkemueller-hsh wiebkemueller-hsh added improvement Something which improves an existing feature in some way (UX, UI, Design, Functionality) and removed feature Something which is a new feature or big improvement labels Oct 4, 2023
@abias abias moved this to Ready for REVIEW in Boost Union Planning Board Nov 2, 2023
@danowar2k
Copy link
Collaborator Author

I said somewhere else that it would perhaps be wise to wait to implement this feature until we have some more people that would benefit from this change. So, if anyone approves of this change, I'd ask of you to inform us about your specific use case so that we get a broader understanding of the utility of this.

At our institution, we currently don't need this change because at the moment, we just configure Boost Union and do not need to override any of it's settings. We also decided that it was ok to define our own theme but not have a separate settings page for it, but just configure Boost Union.

Therefore my older PR #250 wasn't needed by us at all.

@abias abias moved this from Ready for REVIEW to on hold in Boost Union Planning Board Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child theme support Something which helps the creation and management of Boost Union child themes improvement Something which improves an existing feature in some way (UX, UI, Design, Functionality)
Projects
Status: ON HOLD
Development

Successfully merging a pull request may close this issue.

3 participants