Use config for color module palettes

[docwilmot]

Color module uses an array in a color.inc file to store palettes. If we used config, people could import new palettes to their themes, have multiple custom palettes, and easily export color settings from dev to prod.

Could this work before 2.0? It would be an API change?

---

Advocate @docwilmot

[klonos]

This makes so much sense! We'd need to support having /config directories in themes (does this already work BTW?). ...but would that then also require themes to have install/uninstall hooks (so that the config files are removed when removing a theme)? Which might mean that we'd need to introduce an "Uninstall themes" tab in the UI.

[herbdool]

Themes already store some things in config files (which you can't actually uninstall) so the basics are there.

Maybe you could just build the array by calling config->get

[docwilmot]

@herbdool its not the technical stuff I'm concerned about but the API change for contrib. Although, practically speaking, all contrib with color support were made by me 😉.

[herbdool]

@hosef do you want to weigh in on this? I think converting this to config makes sense. Would make color module more robust.

Perhaps its possible to deal with this in an upgrade hook to make it backwards compatible.

[hosef]

Currently, the Color module has a function called color_get_info() that wraps all the logic to get the settings from color.inc and all other code in the module uses that function to get the settings. I think it would be fairly easy to change that one function to try looking up the settings in the theme config before checking color.inc. With this change, we could also mark the usage of color.inc as deprecated and themers could put their color settings in the .info file like any other theme setting.

[herbdool]

Good idea @hosef

[klonos]

Yep, makes sense @hosef 👍

[docwilmot]

Proof of concept.

• Saves a JSON version of a theme's color.inc in config when the theme is installed
• Allows saving custom color palettes.

This doesnt yet:

• Allow themes to ship with a config file to sync configs from themes to active. Couldnt decide if we should have a theme config folder, or if we should scan the color folder for configs, or if a config folder within the color folder, etc.
• Allow to edit or delete saved custom color palettes via UI.

[docwilmot]

The code for this was updated a while back, but needs some explanation. With this PR, color_get_info() has been updated to look for color info both in config and in color.inc:

• existing themes using color.inc files would work normally, color being pulled from color.inc. Once the themes setting is saved a new themeNAME.color.default.json file is created in active config. Backdrop looks for this file first, and then checks for a color.inc, so future reads for color info will be from this config file.
• new themes can ship with a themeNAME.color.default.json file, and this will be synced to config and be the source of all color info whether there is a color.inc or not.
• themes or module contrib can ship with additional themeNAME.color.STRING.json files that can add new color presets (detail later). These will be merged by color_get_info(), so once theme settings are saved, all theme color data from all configs will end up saved in themeNAME.color.default.json. There may be some debate as to whether we should keep those separate.
• what would have been really nice (planned in my mind all along) would be if contrib modules could provide color scheme presets for their own CSS files. Say you build a new slider but would like for it to work well and look good with all Basis color presets (+/- provide your own). But alas this may not work, as color matching is only done on a CSS file that is declared in the ``themeNAME.info` file! Would perhaps be helpful to provide a hook that adds contrib CSS files to the ones that Color module parses?

Re additional color files, don't know how helpful this would be but it was just 3 additional lines to enable the functionality so it's in there. So if you found a nice color scheme you wanted to share, you can create a new color config file. For example (and this probably qualifies as a bug) our new front page cards have a cards.css file, but its not listed in Basis color.inc css array. The following config file now allows Backdrop to dynamically change the card background. It declares a new color field (cardbackground), sets a default (the base color that Color module will replace) and one scheme (the scheme will automatically show up in Basis settings) and if if you switch Basis to that scheme, card backgrounds change to #006600. Note we use this config to add cards.css to the list of CSS file targets.

{
    "_config_name": "basis.color.cards",
    "fields": {
        "cardbackground": "Card background"
    },
    "schemes": {
        "default": {
            "colors": {
                "cardbackground": "#f2f2f2"
            }
        },
        "card_scheme": {
            "title": "Card scheme",
            "colors": {
                "header": "#20252e",
                "base": "#0074bd",
                "slogan": "#000000",
                "titleslogan": "#fffffe",
                "hovermenu": "#114a75",
                "menutoggle": "#bcbbbb",
                "bg": "#ffffff",
                "text": "#000001",
                "link": "#0073bd",
                "borders": "#9f3838",
                "formfocusborder": "#43afe4",
                "primarytabs": "#586172",
                "primarytabstext": "#010000",
                "buttons": "#dee2ea",
                "footerborder": "#bbbbbc",
                "footer": "#fffeff",
                "footertext": "#000100",
                "cardbackground": "#006600"
            }
        }
    },
    "css": [
        "cards.css"
    ]
}

And that's all. Although probably not a very used feature, but update color to new standards.

[docwilmot]

Could I get a milestone and/or review on this please? Tests pass so far.

[klonos]

No idea why this was assigned to me 🤷🏼

The issue already has the milestone candidate - minor label. As per our documentation, in order to get the actual milestone set for a minor version, we also need an advocate. @docwilmot (or others) if you are advocating for this feature for the next minor (currently 1.27.0, slated for Jan 15, 2024), then please edit the issue summary to indicate that, and we can then take it from there.

Also, does this depend on #6122 in any way? (or the other way around?)

[klonos]

OK, I have some questions (but I might simply need to spend more time testing this in order to get a better understanding).

1. Why do we need to keep color.inc? If we provide an upgrade path from that to using only config instead, then couldn't we ditch it? Having multiple ways to do the same thing can become confusing, it complicates code, as well as our documentation. I was thinking that this could work along these lines:
   • if we detect a color.inc file provided by a theme, we convert it to config, then we ignore that color.inc file + throw a warning in the status page and/or the theme settings page.
   • in Backdrop 2.x, we can consider removing the code that provides the functionality to convert color.inc files - the expectation will be that all themes would have switched to config by then.
2. In the current implementation of the feature in the PR, if the same color scheme happens to be defined in color.inc + the default config file + any additional config file(s), which one gets precedence/priority? Which one should be the source of truth, and how can a themer know that?

@docwilmot can you please outline various scenarios to test this? What would be a simple use case? Anything more complex?

[hosef]

1. We should probably keep it for backwards compatibility. Currently, if you add a new color to an existing theme through color.inc it will show up in the UI right away. That should continue to happen for themes with a color.inc.

[hosef]

I have been looking through this for a bit, and I think that the color.inc file should continue to be the source of truth if it exists. In the PR, if it finds a config file it will ignore the color.inc file in favor of using the config file. I can easily imagine a scenario where someone might spend hours trying to find what is going on.

• Developer creates a color.inc file and starts adding settings
• Developer sees that as settings are added, the UI updates automatically
• Developer presses the save button 1 time to test if it is working
• Developer spends hours trying to figure out why changes to color.inc are no longer showing up in the UI

[docwilmot]

we also need an advocate

I've advocated.

Also, does this depend on https://github.com/backdrop/backdrop-issues/issues/6122 in any way?

Note #6122 is already merged, its only open for docs. And yes this would depend on that.

Why do we need to keep color.inc?

Because color.inc is provided by existing themes, not core, and at the time this PR this is merged, no existing theme will have its color info in config yet. And maybe even for years to come few theme devs will bother to use config either.

if we detect a color.inc file provided by a theme, we convert it to config, then we ignore that color.inc file + throw a warning in the status page and/or the theme settings page.

This is essentially what this PR does: check for color info in either color.inc or config, ignore color.inc if a config is found. This way theme devs can either convert their themes to config, and delete the color.info file and create a new release, or if theyre nervous about this new-fangled system, leave both in, let Backdrop sort out which is used. No warning message, then, since its not a mistake, and requiring end-users to"hack core" and delete the color.inc file manually to get rid of a persistent warning would be excessive.

I think that the color.inc file should continue to be the source of truth if it exists.

I think this is contrary to what we do with other theme settings though: we have a hard-coded settings[]='' in theme info files, but once you've saved your theme settings, Backdrop uses the config file thereafter. I think this is the reasonable way to go because the opposite defeats the purpose of portability of config.

Developer spends hours trying to figure out why changes to color.inc are no longer showing up in the UI

Perhaps re-merge the two on cache flush? Or maybe add a message "Color info is being read from config" on settings save might be simpler.

[klonos]

... No warning message, then, since its not a mistake, ...Or maybe add a message "Color info is being read from config" on settings save might be simpler.

Yes, I like that 👍🏼 ...same as we are throwing a message to let people know that we have flushed caches when they switch themes.

However, since this is not a one-time thing like cache clear, the message should be in the settings page of the theme constantly instead of shown only once during save. This should be more like the info message we are showing when the Color module is disabled and a theme supports it:

[hosef]

I guess my main question is, how do theme developers update this after the theme settings are saved? Because the settings need to reflect what is in the CSS files provided by the theme, then any change to the CSS would require changes to the settings. Suppose the theme developers want to add a new setting, or change the original color in CSS, or change a preset that they provided.

My guess is that we have 2 options. The first is that we would need to allow theme developers to put update hooks in the theme. I suspect that would almost guarantee that no theme developers would actually use the system. The second would be to make a UI for the color module settings. If we are going to go through the effort to do that though, I would recommend making something more comprehensive in contrib instead.

[docwilmot]

Not quite following you @hosef. Are you meaning for a theme out in use and the dev has made updates in a new release? As I understand it that dev would have needed to update the color.inc in that case, and would now instead update the json file instead?

[docwilmot]

I think this is ready for another review please.

[argiepiano]

@docwilmot, can you outline ways to test this?

[argiepiano]

Some prelim testing. I created a new custom palette with a name.

• I see the new color config file in the config folder called THEME.color.default.json, which includes new palette 👍🏽
• It's strange that new palettes are included in the THEME.color.default.json file. I had assumed that it would create separate config files for each new palette, making it easier to import/export
• More serious: I don't see the new color config file listed in admin/config/development/configuration/single/export. Can't export or import
• And, if I uninstall the theme, the THEME.color.default.json config file is not removed

[docwilmot]

Thanks for testing, I believe all above should be addressed now.

@docwilmot, can you outline ways to test this?

In a theme that is color-enabled, when you go to the color settings form:

• When you select "Custom" color scheme a name field should pop up, which will allow you to save your new color scheme
• Same happens if you modify a color on a theme-provided color scheme
• New custom schemes will be editable
• Custom schemes will save to separate THEME.color.SCHEME_NAME files in config
• Enabling a theme should create a THEME.color.default file in config
• For existing installs, there is no upgrade hook to create THEME.color.default file in config; the theme will read from the color.inc. But just saving the appearance page should do it as well.
• Disabling a theme should delete all color configs
• All schemes should be exportable

[docwilmot]

I'm thinking in the schemes list custom schemes could use a suffix to show theyre custom? my-scheme - (custom) for example. Not sure.

[argiepiano]

More testing here. This is working great! Thanks for fixing my above comments.

Additional ones:

1. When I select Custom, enter the name of the new scheme, and click save without changing any color, the new scheme is not selected. I have to use the select input widget once again to choose the newly created scheme. Is there any way to select the newly created scheme in the submit handler? (BTW, if you do change a color in the new scheme before saving, it does get selected)
2. This is more of a hypothetical question: I noticed all the json files generated for the schemes contain a lot of the same elements, which do not change regardless of the changes you make to the colors. Those are: css, fields, and blend_target. Wondering if it's necessary to save this same info every time? Seems redundant. I realize this may mean splitting the default file as well, perhaps into two json files: basis.color.settings.json and basis.color.SCHEME.json (and SCHEME may be default as well).

Also, I realize that there is another json file: basis.settings.json which contains a different set of elements (this already existed and is separate from this issue). But perhaps instead of my suggested basis.color.settings.json we could store fields, css and blend_target in that json file, since these do not change with each scheme?

[indigoxela]

@docwilmot what does this change mean to contrib (or custom) themes with color module support (and color schemes, currently in code)?

They all need to get updated, am I getting this right?

And as themes can't have update hooks - what would have to happen to switch those from php code to json config?

[docwilmot]

When I...click save without changing any color, the new scheme is not selected

Color decides which scheme is active by checking the current palette. If you simply saved an existing scheme under a new name, Color might calculate that the old scheme is still active because its exact palette is active. I'll try and figure how to get around that.

But perhaps...we could store fields, css and blend_target in that json file, since these do not change with each scheme

This makes sense, will look into that

They all need to get updated, am I getting this right?

No they wont; Color module will still read from an existing color.inc file as normal.

And as themes can't have update hooks - what would have to happen to switch those from php code to json config?

I've thought about this, and @hosef questioned similar.

• If a theme uses config (with a config folder and x.color.x.json files), then new installs will just run normally of course with the new system.
• If an existing install updates to the new version of a currently installed theme where this new version has a new config folder (site owner just swaps out the old theme directory), nothing new will happen since config is only installed to active on enabling a new theme, and the theme will continue to read from color.inc. But I think "nothing happens" is preferable with themes though, even without this PR. You dont want your site suddenly looking different unless you want to. In this case, if you want to, you disable and re-enable the theme to get new config, or import config, or copy the config files to your active folder (or whatever process you use).
• In the future, when a site is already using config, and updates to the latest version of the theme (with new color configs in the config folder) I suspect you'd need to import the new configs manually as well, unless we build in some method of checking if there are unimported configs and prompt the user to use import.
• Maybe other scenarios

[docwilmot]

Updated PR:

When I...click save without changing any color, the new scheme is not selected

This should be fixed.

But perhaps...we could store fields, css and blend_target in that json file, since these do not change with each scheme

Now saving all color info in the existing theme settings file color array (which seems more sensible). Custom schemes now only have a schemes array (and a scheme_config_name which is there for hook_config_info() to find these files). Custom color config files can store more than one scheme.

[stpaultim]

Added this to 1.28 milestone, since it has an advocate.

[stpaultim]

@docwilmot I did some testing tonight. May need to come back to this and do some more.

1. This is what happens when I try to name my custom scheme "Tim's Theme".

2. I notice that if you edit the "custom" scheme you can give it a new name. But if you edit an existing scheme it get's an automatic name that can't be changed in the UI.

3. In one of my earlier tests, I had a custom theme named "custom_7" and "Custom 8". Different syntax for two custom schemes. Sorry, I don't have steps to recreate that yet.

4. I think if we're going to allow users to give custom themes a name, that maybe we shouldn't allow they to edit existing schemes or if we do, provide an option to give them custom names (rather than the automatic names).

[stpaultim]

It seems a bit strange that it counts existing schemes before starting to number custom schemes. The first custom scheme is numbered as 7, because of 6 other provided schemes.