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

'Required' parameter name is confusing #906

Closed
guillaumemolter opened this issue Apr 18, 2016 · 12 comments
Closed

'Required' parameter name is confusing #906

guillaumemolter opened this issue Apr 18, 2016 · 12 comments

Comments

@guillaumemolter
Copy link
Contributor

I think the name/term required is a little confusing. The customizer is mostly about generating forms. In HTML the required attribute has a very specific meaning. This field is required and unless you specify a value for it you won't be able to save.

As a dev I was expecting, based on the param name, to be able to put required=true and make this field compulsory. But it seems that in Kirki required has another meaning, and I think it's confusing.

I know that in the main Kirki context it is very rare (and most of the time a bad idea) to force users to submit a value for field. However in the Repeater control context this is very likely to happen and I'm currently working on implementing this, but I can't use 'required' without making it confusing.

I suggest we rename the required param into conditional ( or dependencies or display_callback or show_if ... ). And use required for Kirki fields and repeater fields as a way to make the field compulsory.

What do you think?

@rahulv3a
Copy link

I never thought of this in this way. But you do have a point. Other names like 'dependencies' make more sense.

But renaming it at this point would break things for a lot users considering when many developers are powering their theme's customizer with Kirki.

@guillaumemolter
Copy link
Contributor Author

guillaumemolter commented Apr 18, 2016

Yes that would definitely be a breaking change...maybe we could support both for a few versions with a deprecation notice when WP_DEBUG is on, to give devs some time to transition.

@guillaumemolter
Copy link
Contributor Author

Also if required is defined and different from true/false we could remap it to the new dependencies param.

@aristath
Copy link
Contributor

Also if required is defined and different from true/false we could remap it to the new dependencies param.

Exactly what I was thinking. This way nothing breaks... As for making people switch to the new naming convention, I can just change it in the docs so that new devs do it the right way, and we can add notices in the error log for others. Eventually they'll migrate.

@guillaumemolter
Copy link
Contributor Author

Sounds good to me! I'll start working on it soon. @aristath which name do you prefer for the new param?

@aristath
Copy link
Contributor

What if we use the active_callback?
If it's an array of arrays with the right keys, then evaluate it in kirki, else just let the customizer API handle it.
Do we really need a separate argument?

@manuelmoreale
Copy link
Contributor

My 2c:
I don't think merging the current required functionality with the active_callback is the best idea.
IMO is better to have separate functions that do one thing and one thing only.
As for the name, a similar functionality in the ACF Plugin is called Conditional Logic.

(Btw @aristath and @guillaumemolter you guys are doing an amazing work with kirki)

@Rayken
Copy link

Rayken commented Apr 19, 2016

I agree with this issue and @manuelmoreale however personally I'm for the 'conditional' name; it makes sense to me. If a field has a condition, it becomes a conditional field which means the given condition(s) have to be met in order for it to show/function!

@guillaumemolter
Copy link
Contributor Author

I agree with @aristath, the active_callback is here to manage conditional display of panels, sections, and controls. Since Kirki's goal is to extend the customizer it actually makes a lot of sense to extend the active_callback param. The core function currently only accept String (filters,a actions and function reference) or anonymous functions so we could extend it to accept an array.

@manuelmoreale @Quaked do you guys see a real issue with this? Something we are missing?

(@manuelmoreale thx but I don't do much, @aristath is the true hero here :-) )

@Rayken
Copy link

Rayken commented Apr 19, 2016

No, ultimately you know what's best. I'm just tossing my thoughts out loud. The only reason "not to" use active_callback for me, personally, is that I immediately think it's to be used with "is_front_page" or maybe "is_single" or even "is_page" and such stuff whereas a condition would be in addition to this, like the active callback is a pre-requisite of the field and a condition is an additional condition really. But yeah if you make an array and we can have both? I don't mind at all!

Or did I get it mixed up? Anyhow, just my thoughts :)

@manuelmoreale
Copy link
Contributor

@guillaumemolter I personally have no problem using the active_callback. My only concern was related to how easy is to document the whole thing.
Having a separate argument makes everything way easier to explain because in most cases passing the array to the required is more then enough and we also have the active_callback for more complex cases.
But again, I'm totally cool with the idea of dropping the required and extend the active_callback.
If makes things easier to maintain going forward I'm more than happy with this solution.

@aristath
Copy link
Contributor

I just submitted a simple patch on the develop branch that makes active_callback accept the same format as the required argument while being 100% backwards-compatible.

Docs will be updated once the next version is released. 👍

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

No branches or pull requests

5 participants