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

Allow 0 as value in multi-context settings #190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugopeek
Copy link

@hugopeek hugopeek commented Apr 6, 2022

What does it do?

This accepts 0 values as non-empty for multi-context settings.

Why is it needed?

When using ClientConfig in multi-context mode, you can't set an actual value of 0 (tested with numberfield and selectbox). This causes the setting to fall back on the global value.

I don't know if this is a bug or a design choice, but in my opinion 0 does not always equate to empty here. So I propose to return 0 as context-specific value, instead of the global fallback.

Remarks

I hope this won't break anyone's existing setup. It's also a little tricky to select a truly empty option in a selectbox, because the following doesn't work:

Global==||Low==0||Medium==1||High==2

In this case, Global is returned if you select the first option. Also a little bug maybe? Because this would work with TVs.. In any case, this does work:

||Low==0||Medium==1||High==2

This renders and empty first option and when selected, the global value is returned. Not the prettiest solution, but at least it allows you to replicate the old behaviour. A reset option similar to the one for TVs would be nice here of course, but that's probably a little too much to ask for ;)

@sebastian-marinescu
Copy link
Contributor

Hi Hugo, nice PR 👍

Just noticed this behavior, in a theme-case:

image

Knowing the behavior after your PR would allow me to "fix" it, and add a description telling the editor that the first blank option is actually the global option (which I'm thinking of introducing there ^).

But certainly this

Global==||Disabled==0||Enabled==1

would be much nicer.

Don't you think we could make Global== possible, by fixing the code that splits this into key/value?
( I think this is the culprit: https://github.com/modmore/ClientConfig/blob/master/assets/components/clientconfig/js/mgr/sections/home.js#L186 )

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.

2 participants