-
Notifications
You must be signed in to change notification settings - Fork 6
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 JavaScript unit tests and refactor #35
Conversation
Changes the function to accept an object, alter that object's prototype, and return it. The caller is then responsible for passing the constructor function to change. This allows the function to be more easily tested since it does not directly mutate global variables (although of course changing the prototype of a global variable will have a a global effect). Also adds `wp` and `_` as parameters to the IIFE to make their usage explicit.
Not currently using real Controls, but it's something.
IIFEs are good tools for encapsulating functionality and to maintain that encapsulation, all inputs and outputs (the exported API) need to be explicit. To simply alias variables, variable assignment is a less cumbersome tool.
`_.clone()` only performs a shallow copy and `_.extend()` is already doing that here.
This does not seem necessary.
Debounced functions are async by nature and harder to test. This rewrites `renderNotifications` as a debounced proxy for `renderNotificationsToContainer`.
1d70453
to
7f3d014
Compare
As part of the changes I simplified two-way data binding a bit (see 16f9762). I'm not sure if the resulting simplified version covers everything that the |
@sirbrillig oh, I didn't consider how I think that --- a/js/widget-form.js
+++ b/js/widget-form.js
@@ -272,15 +272,15 @@ wp.widgets.Form = (function( api, $ ) {
* @returns {object} Property value instance.
*/
createSyncedPropertyValue: function createSyncedPropertyValue( root, property ) {
- var propertyValue, rootChangeListener, propertyChangeListener;
+ var form = this, propertyValue, rootChangeListener, propertyChangeListener;
propertyValue = new api.Value( root.get()[ property ] );
// Sync changes to the property back to the root value.
propertyChangeListener = function( newPropertyValue ) {
- var rootValue = _.clone( root.get() );
- rootValue[ property ] = newPropertyValue;
- root.set( rootValue );
+ var newState = {};
+ newState[ property ] = newPropertyValue;
+ form.setState( newState );
};
propertyValue.bind( propertyChangeListener ); This will ensure that the Does that make sense? |
ca240cb
to
02b0a1c
Compare
This makes sure that data from the DOM is validated before changing the model.
Ah, nice. Don't know why I didn't see that part of the function before. I guess I wasn't clear what was going on in there. I made the changes you suggested and the tests all pass. 👍 (I rebased away the old version.) One last question: In |
Great!
@sirbrillig Humm, I don't have a good answer for you. I can't remember why |
TIL that CSS transitions aren't available in IE < 10. It's amazing how quickly I've come to think of them as natural. Well, I don't know why |
Great! I'll give this PR a test soon and see if there's any issues on my end and then merge. Excellent work here. Really appreciate the depth you went and the insights you have. |
@sirbrillig I found that the widgets weren't getting wp.widgets.formConstructor.foo = wp.widgets.Form.extend({
id_base: 'foo'
}); So there is some redundancy there. In reality they should never be different, so it seems a shame to duplicate. |
I'm not sure I understand. Do you mean, the So |
|
I removed |
Add basic JavaScript unit tests for
wp.widgets.Form
. They can be run usingnpm test
on the command-line (you may need to runnpm install
first to get the tests properly set up).This also makes some changes to the
wp.widgets.Form
class to make the code more concise and more testable. There are also a few bugs that are fixed. Changes include:_.clone
Form
as a module when within CommonJSForm.initialize
to deep merge config with defaultsdefault_instance
with the model in all places exceptForm.getValue
title
property to model inForm.sanitize
Form.model.validate
methodForm.initialize
constructorForm.renderNotificationsToContainer
to allow testingForm.renderNotifications
Form.model.validate
This needs to be tested with a real-world Form to be sure that the above changes do not break anything.
There's also more simplifications that could be made, but I wanted to get a PR up for the purposes of discussion.