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

Add Customizer context #399

Open
wants to merge 86 commits into
base: master
from
Open

Add Customizer context #399

wants to merge 86 commits into from

Conversation

@dlh01
Copy link
Member

dlh01 commented Nov 15, 2015

This is a work-in-progress PR to add a Customizer context. If you want to take it for a spin, try the standalone plugin: https://github.com/dlh01/fieldmanager-beta-customize/.

What's included so far covers most of Fieldmanager's fields and their features, but questions about the implementation and known bugs are below.

Suggestions and feedback are appreciated!

Bugs

  • Fieldmanager_Datepicker doesn't work.
  • Fieldmanager_RichTextArea doesn't work (and I'm not sure what would go into fixing this).
  • Unnecessary _debounce() applies when updating an Autocomplete's hidden field.
  • There's no signal to the user if a field fails validation.
  • Any changes to field values from sanitizing aren't reflected in the form.
  • JS callbacks are not bound to dynamically added sections.
  • Fieldmanager_Colorpicker doesn't work.
  • Inline docs about the architecture are insufficient.
  • Missing unit tests.

Questions

  • Is there a better way to support postMessage settings? Currently, these settings work, but the preview receives a serialized string, and none of Fieldmanager's processing (e.g., validation) occurs beforehand. Replacing jQuery's serialize() with serializing into JSON (which would then be passed to both JS and PHP) could help here.
  • Is there a better way to reinitialize FM sortable fields or label macros than adding event handlers to fieldmanager.js?
  • Is it a problem to manually set the Autocomplete's hidden field value to "", which doesn't happen in other contexts?

Resolves #331.

dlh01 added 3 commits Oct 17, 2015
Autocomplete fields in the Customizer are not likely to be visible when
`fm.autocomplete.enable_autocomplete()` first fires. Rather than adding
another FM-specific event, this triggers `enable_autocomplete()`
whenever an uninitialized autocomplete input is focused.

Bonus: This affects autocomplete fields in widgets and menu items, too.
These handlers allow contexts (immediately, the Customizer context) or
third-party extensions to reinitialize these features after events not
relevant to fieldmanager.js.
@danielbachhuber
Copy link
Collaborator

danielbachhuber commented Nov 16, 2015

💯

dlh01 added 6 commits Nov 22, 2015
This helps ensure the UI Datepicker script loads when adding a
Datepicker field to the Customizer.
This removes the two event handlers that existed only to enable sortable
fields and label macros. Instead, we trigger one event when a Customizer
section containing a Fieldmanager control expands, and have those
features listen for it. This is a little more consistent with other FM
features that listen for FM-related events.
Moves the initialization logic to a function so it can be called during
'fm_customizer_control_section_expanded'.
@dlh01
Copy link
Member Author

dlh01 commented Dec 2, 2015

The last round of changes to this PR should have fixed Datepicker fields and the _debounce() delay on Autocomplete fields. It also fixed display_if, which wasn't working at all.

Is there a better way to reinitialize FM sortable fields or label macros than adding event handlers to fieldmanager.js?

I've answered this question for now as "yes" and added a fm_customizer_control_section_expanded trigger, which other scripts in FM can bind to. This seems a little more consistent to me with other FM JS behavior.

@dlh01
Copy link
Member Author

dlh01 commented Dec 3, 2015

Any changes to field values from sanitizing aren't reflected in the form.

This seems to be an issue with core controls as well, so I've marked it as "done," for now, in that Fieldmanager doesn't introduce it.

@@ -77,5 +77,9 @@ fm.autocomplete = {

$( document ).ready( fm.autocomplete.enable_autocomplete );
$( document ).on( 'fm_collapsible_toggle fm_added_element fm_displayif_toggle fm_activate_tab', fm.autocomplete.enable_autocomplete );
$( document ).on( 'focus',
'input[class*="fm-autocomplete"]:not(.fm-autocomplete-enabled)',

This comment has been minimized.

@danielbachhuber

danielbachhuber Dec 3, 2015 Collaborator

Do we need the prior to implementation approaches in addition to this approach?

This comment has been minimized.

@dlh01

dlh01 Dec 3, 2015 Author Member

I don't think so. But the existing JS predates my involvement with FM, so I'd appreciate a review from you or someone else to confirm.

* Initializes triggers to conditionally hide or show fields.
*/
var init_display_if = function() {
$( '.display-if' ).each(function() {

This comment has been minimized.

@danielbachhuber

danielbachhuber Dec 3, 2015 Collaborator

It seems like this could collide with another class in global scope. Can we narrow scope?

This comment has been minimized.

@dlh01

dlh01 Dec 3, 2015 Author Member

I agree, although because this is existing FM code just relocated, I'd be inclined to determine a better scope in a separate PR. If that sounds OK to you, I'll file an issue.

This comment has been minimized.

@danielbachhuber

danielbachhuber Dec 3, 2015 Collaborator

If that sounds OK to you, I'll file an issue.

Sounds fine

dlh01 added 2 commits Dec 6, 2015
This primarily updates Fieldmanager_Field::_failed_validation() so that
it returns usable data to the Customizer. The particular error string
and the method of alerting users might come in for more updates.
@dlh01
Copy link
Member Author

dlh01 commented Dec 6, 2015

0795c83 updates the behavior in Fieldmanager_Field::_failed_validation() to account for the Customizer, and it provides an alert() to the user if validation fails while attempting to save changes.

At the moment, there doesn't seem to be a standard way to alert users to errors while saving changes in the Customizer. alert() is at least consistent with the "Are you sure" prompt, but it still isn't great, and suggestions for alternate approaches are welcome.

There also doesn't seem to be a great a way to send a message when an error occurs while previewing a setting change. Failing validation will still stop the preview from using invalid values, but it does so silently. Suggestions welcome here, too.

dlh01 added 8 commits Dec 7, 2015
This helps ensure we bind 'fm_customizer_control_section_expanded' to
sections that are dynamically added.

This also changes the previous behavior of binding to only sections with
Fieldmanager controls: If an FM control is dynamically added to a
section that had no FM controls, we need to trigger the event when that
section expands.

Binding to when controls are added, rather than sections, isn't enough
because controls can be added without being assigned to sections.
Eventually, it might be more efficient to try binding to a section only
when a Fieldmanager control is added to it.
This follows other FM error messages, e.g. "You may be able to use your
browser's back button" and "Please check your code and try again."
Instead, reserialize only those controls containing the changed element.
- After dropping a sortable field, pass the available element directly
  to reserializeControlsContainingElement()
- Call reserializeEachControl() after removing fields or values, because
  they no longer exist and so the control doesn't contain them.
When previewing or saving a Fieldmanager field in the Customizer, we now
use the jquery.serializeJSON][1] library to serialize the FM control
into a JavaScript object. The object is then set as value of the
control's setting.

This allows us to send an object both to PHP, as part of a 'refresh'
setting, and to JS, as part of a 'postMessage' setting. Previously each
received a string from jQuery's `serialize()`, which remains the
fallback behavior.

[1]: https://github.com/marioizquierdo/jquery.serializeJSON
@dlh01
Copy link
Member Author

dlh01 commented Jan 26, 2016

Is there a better way to support postMessage settings?

01893e7 adds the serializeJSON library for converting the value of an FM field inside a control into an object, which can then be handled more easily in PHP or JS.

dlh01 added 3 commits Jan 30, 2016
stripslashes_deep() matches existing submenu saving behavior.
This lets the context manage the field object, rather than passing the
field to both the context and the control. It also lets us use the
context's own API for rendering instead of echoing it ourselves.
dlh01 added 20 commits Aug 19, 2016
Adds a check for the `'customize_validation_'` filter in
`Fieldmanager_Customize_Setting::_preview_filter()` to avoid an infinite
loop, much like the check for the `'customize_sanitize_'` filter.
Moves the logic for converting the `jQuery.serialize()`'d string of a
field value into the array or string to save.
When the context can use the Customizer's validation framework, and when
the default callbacks are added to FM Customizer settings, any invalid
submissions will have already been rejected before sanitizing occurs.

But, we still need to check for invalid values in the sanitizing
callback just in case, for example, the method is called on its own or
the validation callback is not hooked to the Customizer setting. If a
field value is invalid, we need to reject the update with a
Customizer-approved `WP_Error` or `null` before Fieldmanager's native
validation routines (such as throwing an Exception or calling
`wp_die()`) kick in.
Adds a data provider, `data_field_debug()`, for testing the same method
while `Fieldmanager_Field::debug` is `true` or `false`, because the
`$debug` property affects Fieldmanager's response to invalid
values. These tests currently fail when `$debug` is false.
Adds a handler to the Customizer context that intercepts `wp_die()` and
throws it as an exception which the default validation callback can catch.
…ripts()` priority

This method was previously hooked to an earlier priority than when it
was actually called, which is invalid under `WP_Hook`. See
https://core.trac.wordpress.org/ticket/38011.
'Customizer' for the thing itself.
dlh01 added a commit to dlh01/fieldmanager-beta-customize that referenced this pull request Oct 12, 2016
Includes all of the changes from the Pull Request into Fieldmanager (see
alleyinteractive/wordpress-fieldmanager#399)
except for the unit tests, which will take some more work to decouple.
@dlh01
Copy link
Member Author

dlh01 commented Oct 17, 2016

The proposed Customize context is now available as a standalone plugin that you can use alongside a stable version of Fieldmanager: Check out Fieldmanager Beta: Customize (props @jameswburke for reviewing the initial pull request).

Screenshots, installation instructions, details about how the plugin integrates with Fieldmanager, and steps to activate a demo are all available in the README.

If you're interested in using the Customize context, please try the plugin. Feedback, bug reports, and pull requests are all welcome at the repo.

Plugin development will continue much like a feature plugin in WordPress core. Improvements to the plugin based on testing will, hopefully, strengthen this pull request in turn.

@mboynes mboynes modified the milestones: 1.2.0, later Oct 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.