[wip] Add temperature example to App framework. #276

albertosantini · 2012-09-21T13:20:42Z

This example demonstrates how to create a simple temperature converter using Model and View components, highlighting validation and events features.

ericf · 2012-09-21T21:05:50Z

Cool! I think this example will be good for showing the basic interaction between Models and Views in a way that will be really helpful for people.

ericf · 2012-09-21T21:08:45Z

Looking through the code I see one major thing that jumps out at me. It appears that the View is doing some of the Model's work. Could you refactor this such that the Model keeps its attribute values in sync when one of them changes? To avoid an infinite loops, you'll want to utilize the src option when calling set().

Also, instead of using the keydown event, I'd recommend switching to valuechange which is provided by the event-valuechange module.

Change keydown event with valuechange. Move convert methods from the view to the model. Rename convert methods of the view to update. Add a few comments and rename variables just to be more clear. Fix a typo in app-temperature.mustache. Add event-valuechange in modules and tags in component.json.

albertosantini · 2012-09-22T07:39:02Z

Thanks Eric for the insightful advices. I hope I implemented them correctly.

I didn't get src option.

For instance, if I add it to the set

this.set('celsius', this.round((value - 32) * 5 / 9), {src: 'fahrenheit'});

I didn't understand how to use it.

juandopazo · 2012-09-22T15:19:43Z

@albertosantini the src property is almost a standard in YUI code to identify who initiated the set() action and prevent infinite loops or other similar situations. Eric proposes that every attribute representing a temperature unit has a listener for its change event that modifies the other units. For example:

_onCelsiusChange: function (e) {
  this.set('kelvin', this._celsiusToKelvin(e.newVal));
  this.set('fahrenheit', this._celsiusToFarenheit(e.newVal));
}

However, if the afterKelvinChange event listener changed the celsius attribute, you'd have an infinite loop. So what you do is "mark" the setting action as "this came from a change listener" so that it doesn't change other attributes. For example:

var TRANSFORM = 'transform';
/* ... */
_onCelsiusChange: function (e) {
  if (e.src !== TRANSFORM) {
    this.set('kelvin', this._celsiusToKelvin(e.newVal), {src: TRANSFORM});
    this.set('fahrenheit', this._celsiusToFarenheit(e.newVal), {src: TRANSFORM});
  }
}

This way the View will only have to:

Listen to the change event of the Model to re-render
When the user changes the value of an input, change the value of the corresponding unit in the model

albertosantini · 2012-09-22T17:26:44Z

@juandopazo Thanks for the details.

I think I don't understand the last detail: I cannot figure out where I should write
model.on('celsiuschange', _onCelsiusChange);

In the initializer of the view?
Using myTemperatureModel in the 'main' (I mean outside the model and the view instance)?
Or there is a default method I should override in the model (I tried _onCelsiusChange)?

juandopazo · 2012-09-22T18:55:02Z

In the Model's initializer, listen for the change event of each unit and change the others:

/* ... */
initializer: function () {
  this.after('celsiusChange', this._afterCelsiusChange);
  this.after('fahrenheitChange', this._afterFahrenheitChange);
  this.after('kelvinChange', this._afterKelvinChange);
},
_afterCelsiusChange: function (e) {
  if (e.src !== TRANSFORM) {
    this.set('kelvin', this._celsiusToKelvin(e.newVal), {src: TRANSFORM});
    this.set('fahrenheit', this._celsiusToFarenheit(e.newVal), {src: TRANSFORM});
  }
}
/* ... */

In the View's initializer, listen for the change event and update the DOM:

initializer: function () {
  var temperature = this.get('model');
  temperature.after('change', this.updateView, this);
  this.clear();
}

juandopazo · 2012-09-22T18:55:29Z

src/app/docs/app/partials/app-temperature-js.mustache

+
+    // Events are attached lazily.
+    // They will be attached on the first call to getting the container node.
+    myTemperatureView.get('container');


Shouldn't this be in the render method of the View?

Add change events to the model initializer. Add src property to the sets of the model to avoid recursion. Move round method to the view. Move the init of the view events to the view initializer. Add a few comments.

albertosantini · 2012-09-23T10:02:44Z

I think I implemented all the suggestions. :)

Anything else?

As usual I learnt a lot of things.

Thanks Juan and Eric.

juandopazo · 2012-09-23T14:48:37Z

src/app/docs/app/partials/app-temperature-js.mustache

+            // the `set` method.
+            _CONVERSION: {
+                value: 'conversion'
+            }


We usually use a variable to avoid typos, but it can be just a string. Using an attribute is probably overkill and maybe confusing, you could use a class property TemperatureModel.CONVERSION if you don't want it to be a private variable.

albertosantini · 2012-09-23T15:56:23Z

I removed the fake attribute _CONVERSION used with the src.
I used the built-in property clientId.

It seems to me elegant, because there is not need to add any variable.

Gracias Juan.

juandopazo · 2012-09-23T15:59:36Z

Don't thank me. I never get around to writing examples. It's easier to criticize. 👅

Kudos on the initiative!

At the moment they fail, because I do not know how to simulate valuechange.

albertosantini · 2012-09-23T17:35:25Z

I added the tests for the example, but they fail, because I do not know how to simulate valuechange.

Then I found a bug in the example: if I type, for instance, '3' in the fahrenheit input field, the conversion is displayed; then I type backspace and the fields are cleared; if I retype '3', the conversion is not done because there is not a change in the model (fahrenheit attribute contains 3).

So I added temperatureModel.reset() after clearing the fields in temperatureUpdate(): now the attributes should contain the initial values (NaN), but the fahrenheit attribute contains the value 3.

What am I missing for the valuechange simulation and the reset of the model?

ericf · 2012-09-24T13:33:50Z

src/app/docs/app/partials/app-temperature-js.mustache

+    // Usually the TemperatureModel and TemperatureView classes should be
+    // implemented in separate files as modules. For the sake of the simplicity
+    // those classes are here as local variables and not in a namespace as
+    // Y.TemperatureModel and Y.TemperatureView.


I actually think it's easier for people to distinguish between classes and variables, when the classes hang off the Y object.

albertosantini · 2012-09-24T14:51:52Z

@ericf Done.

Any hint for valuechange in the tests and the model reset?

I think the problem with the reset is the default value NaN.

ericf · 2012-09-24T16:54:21Z

src/app/docs/app/partials/app-temperature-js.mustache

+                // clientId is an unique ID automatically generated for
+                // identifying model instances. Any constant value would be
+                // fine.
+                clientId = this.get('clientId');


I liked the old idea of using the string "conversion" as the event source.

ericf · 2012-09-24T20:49:39Z

I'll have to look into that more. Let me do that today and I'll get back to you.

The model is not reset if the attribute doesn't pass the validation process. If the default value is out of scope of the validation, it is impossible to reset the model.

Adding the focus to the node and waiting a few milliseconds, just to give time to the polling of the valuechange to catch the change, the tests are fine.

ericf · 2012-09-27T15:51:20Z

src/app/docs/app/partials/app-temperature-js.mustache

+
+        // This is a method used in the initialization of the attributes.
+        _unitSetter: function (val) {
+            return Y.Lang.isNumber(val) ? val : NaN;


I think this is a bit confusing… I'm curious what you're trying to do here. Also, technically NaN is a number :-/

console.log(typeof NaN); // => "number"

ericf · 2012-10-08T14:31:41Z

@albertosantini I wanted to give you an update on this…

Last week I pulled this down locally, ran the example, and I wasn't happy with the UX. This is not your fault, rather I think you've highlighted a very common use case which the YUI App Framework doesn't have a great answer for. I'd like us to really nail a solid, robust solution/pattern that can applied anytime someone gets into a situation like this.

I have a few varieties of changes to this example locally, none of which I think are solid enough yet. But I'll post some ideas here this week and we can continue to discuss and determine a bullet-proof solution for this type of data and user interaction.

Again, I think we really need this example, I just want to make sure we're providing the best possible solution/pattern.

albertosantini · 2012-10-08T14:47:37Z

I agree. Same feelings here.

I think the long recycle of the pull and a few comments in the outdated diffs reflect what you described.

I am open to any suggestion. :)

albertosantini · 2012-12-08T08:57:20Z

I read Jeff's post about using after() event listeners for reacting to attribute value changes:

http://fromanegg.com/post/37222972936/using-after-event-listeners-to-react-to-attribute

Do you think is it a feasible approach (vs. event-valuechange)?

Maybe it doesn't apply because the example uses Model and View approach (with Attribute built-in event architecture), but it would be interesting to release the example. :)

ericf · 2013-01-24T22:40:22Z

I think we should approach this with the data binding features that are brewing: #386 I think this will also serve as a good test to make sure the data binding features can handle this use case. /cc @lsmith

clarle · 2014-05-18T03:37:32Z

Hi @albertosantini,

I'm closing this due to a bit of a lack of activity, but I'd definitely love to see this continue if you decide working on it.

We can definitely always have it published as a blog post instead, if you want to talk about some of the work you've done here.

Let me know, and cheers!

albertosantini · 2014-05-18T06:11:40Z

Hello @clarle.

Without any news on event side or app framework, the state of art of the example would not be too much different from the proposed.

I agree about closing it.

Cheers!

Add temperature example to App framework.

6dcca80

ghost assigned ericf Sep 21, 2012

juandopazo reviewed Sep 22, 2012
View reviewed changes

Better refactoring of TemperatureModel.

5b8dbf1

Add change events to the model initializer. Add src property to the sets of the model to avoid recursion. Move round method to the view. Move the init of the view events to the view initializer. Add a few comments.

juandopazo reviewed Sep 23, 2012
View reviewed changes

Remove _CONVERSION attr and use clientId prop.

2f59530

Add temperature example tests.

8b6f01e

At the moment they fail, because I do not know how to simulate valuechange.

ericf reviewed Sep 24, 2012
View reviewed changes

albertosantini added 2 commits September 24, 2012 16:25

Put the model and the view classes in Y namespace.

e798619

Use only one event handler for unit change.

c9189b5

ericf reviewed Sep 24, 2012
View reviewed changes

albertosantini added 2 commits September 24, 2012 19:12

Use conversion string in src.

d040426

Refactor unit conversion.

e1f461c

Fix a typo.

6db4151

albertosantini added 3 commits September 25, 2012 17:36

Fix the problem with reset using a setter method.

3e8e974

The model is not reset if the attribute doesn't pass the validation process. If the default value is out of scope of the validation, it is impossible to reset the model.

All tests passed.

5aa0acf

Adding the focus to the node and waiting a few milliseconds, just to give time to the polling of the valuechange to catch the change, the tests are fine.

parseFloat has not the radix as second argument.

9e97c01

ericf reviewed Sep 27, 2012
View reviewed changes

Remove _unitSetter and the validator of the attrs.

ac85cc6

clarle closed this May 18, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Add temperature example to App framework. #276

[wip] Add temperature example to App framework. #276

albertosantini commented Sep 21, 2012

ericf commented Sep 21, 2012

ericf commented Sep 21, 2012

albertosantini commented Sep 22, 2012

juandopazo commented Sep 22, 2012

albertosantini commented Sep 22, 2012

juandopazo commented Sep 22, 2012

juandopazo Sep 22, 2012

albertosantini commented Sep 23, 2012

juandopazo Sep 23, 2012

albertosantini commented Sep 23, 2012

juandopazo commented Sep 23, 2012

albertosantini commented Sep 23, 2012

ericf Sep 24, 2012

albertosantini commented Sep 24, 2012

ericf Sep 24, 2012

ericf commented Sep 24, 2012

ericf Sep 27, 2012

ericf commented Oct 8, 2012

albertosantini commented Oct 8, 2012

albertosantini commented Dec 8, 2012

ericf commented Jan 24, 2013

clarle commented May 18, 2014

albertosantini commented May 18, 2014

[wip] Add temperature example to App framework. #276

[wip] Add temperature example to App framework. #276

Conversation

albertosantini commented Sep 21, 2012

ericf commented Sep 21, 2012

ericf commented Sep 21, 2012

albertosantini commented Sep 22, 2012

juandopazo commented Sep 22, 2012

albertosantini commented Sep 22, 2012

juandopazo commented Sep 22, 2012

juandopazo Sep 22, 2012

Choose a reason for hiding this comment

albertosantini commented Sep 23, 2012

juandopazo Sep 23, 2012

Choose a reason for hiding this comment

albertosantini commented Sep 23, 2012

juandopazo commented Sep 23, 2012

albertosantini commented Sep 23, 2012

ericf Sep 24, 2012

Choose a reason for hiding this comment

albertosantini commented Sep 24, 2012

ericf Sep 24, 2012

Choose a reason for hiding this comment

ericf commented Sep 24, 2012

ericf Sep 27, 2012

Choose a reason for hiding this comment

ericf commented Oct 8, 2012

albertosantini commented Oct 8, 2012

albertosantini commented Dec 8, 2012

ericf commented Jan 24, 2013

clarle commented May 18, 2014

albertosantini commented May 18, 2014