Updating the extjs implementation #106

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
4 participants
@boushley
Contributor

boushley commented Feb 23, 2012

Fixes #64

@boushley

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 23, 2012

Contributor

This isn't ready to be merged, but to aide in commenting on the work and getting it ready we've created the pull request now.

Contributor

boushley commented Feb 23, 2012

This isn't ready to be merged, but to aide in commenting on the work and getting it ready we've created the pull request now.

+ setText: function (text) {
+ this.html = this.tpl.apply({text:text});
+ },
+ html: '<a id="clear-completed">Clear Completed</a>',

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 23, 2012

Owner

Is this really needed, since it's set in the function above anyway?

@sindresorhus

sindresorhus Feb 23, 2012

Owner

Is this really needed, since it's set in the function above anyway?

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 23, 2012

Contributor

Probably not... I'll experiment with removing it.

On Thu, Feb 23, 2012 at 2:20 PM, Sindre Sorhus
reply@reply.github.com
wrote:

@@ -0,0 +1,15 @@
+Ext.define('Todo.view.TaskCompleteButton' , {

  •    extend: 'Ext.Component',
  •    alias : 'widget.completeButton',
  •    tpl: new Ext.XTemplate('{text}', {compiled: true}),
  •    setText: function (text) {
  •        this.html = this.tpl.apply({text:text});
  •    },
  •    html: 'Clear Completed',

Is this really needed, since it's set in the function above anyway?


Reply to this email directly or view it on GitHub:
https://github.com/addyosmani/todomvc/pull/106/files#r482657

@boushley

boushley Feb 23, 2012

Contributor

Probably not... I'll experiment with removing it.

On Thu, Feb 23, 2012 at 2:20 PM, Sindre Sorhus
reply@reply.github.com
wrote:

@@ -0,0 +1,15 @@
+Ext.define('Todo.view.TaskCompleteButton' , {

  •    extend: 'Ext.Component',
  •    alias : 'widget.completeButton',
  •    tpl: new Ext.XTemplate('{text}', {compiled: true}),
  •    setText: function (text) {
  •        this.html = this.tpl.apply({text:text});
  •    },
  •    html: 'Clear Completed',

Is this really needed, since it's set in the function above anyway?


Reply to this email directly or view it on GitHub:
https://github.com/addyosmani/todomvc/pull/106/files#r482657

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

Alright, I got the button working and removed this extra code.

@boushley

boushley Feb 24, 2012

Contributor

Alright, I got the button working and removed this extra code.

+ '<li>',
+ '<div class="view">',
+ '<input type="checkbox" {[values.checked ? "checked" : ""]} />',
+ '<span class="{[values.checked ? "checked" : ""]}">{label}</span>',

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 23, 2012

Owner

The label shouldn't get a class. There should instead be a class on the <li> called done, when it's checked.

@sindresorhus

sindresorhus Feb 23, 2012

Owner

The label shouldn't get a class. There should instead be a class on the <li> called done, when it's checked.

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

Good catch, I made that change, and changed it to a label instead of a span.

@boushley

boushley Feb 24, 2012

Contributor

Good catch, I made that change, and changed it to a label instead of a span.

+ '<ul id="todo-list"><tpl for=".">',
+ '<li>',
+ '<div class="view">',
+ '<input type="checkbox" {[values.checked ? "checked" : ""]} />',

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 23, 2012

Owner

Would be nicer to use the if template tag than executing arbitrary code in the template.
<tpl if="checked">checked</tpl>

@sindresorhus

sindresorhus Feb 23, 2012

Owner

Would be nicer to use the if template tag than executing arbitrary code in the template.
<tpl if="checked">checked</tpl>

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

Noted, I also switched the class assignment to be the same way.

@boushley

boushley Feb 24, 2012

Contributor

Noted, I also switched the class assignment to be the same way.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 23, 2012

Owner

Some notes:

  • The footer is inside the main container, it should be below it.
  • You probably know, it's missing the mark all as complete checkbox
  • You should trim the input for whitespace before adding it to make sure it's not possible to add todos with only whitespace.
  • Double clicking the todo doesn't activate the edit mode. And the delete button doesn't do anything.
  • In TaskCompleteButton new Ext.XTemplate( is used, but in Ext.create('Ext.XTemplate' if there isn't any difference, it should be consistent.
  • In the function onStoreDataChanged in Tasks.js; can you use a template for the count and checkedCount?
  • Remove the focus outlines
  • There need to be a space between the checkbox and label in a task
  • The clearCompleted button only shows if one or more of the todos are checked when the app is loaded
Owner

sindresorhus commented Feb 23, 2012

Some notes:

  • The footer is inside the main container, it should be below it.
  • You probably know, it's missing the mark all as complete checkbox
  • You should trim the input for whitespace before adding it to make sure it's not possible to add todos with only whitespace.
  • Double clicking the todo doesn't activate the edit mode. And the delete button doesn't do anything.
  • In TaskCompleteButton new Ext.XTemplate( is used, but in Ext.create('Ext.XTemplate' if there isn't any difference, it should be consistent.
  • In the function onStoreDataChanged in Tasks.js; can you use a template for the count and checkedCount?
  • Remove the focus outlines
  • There need to be a space between the checkbox and label in a task
  • The clearCompleted button only shows if one or more of the todos are checked when the app is loaded
@boushley

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

@sindresorhus In your notes you mention "The clearCompleted button only shows if one or more of the todos are checked when the app is loaded" that is the desired behavior isn't it?

Contributor

boushley commented Feb 24, 2012

@sindresorhus In your notes you mention "The clearCompleted button only shows if one or more of the todos are checked when the app is loaded" that is the desired behavior isn't it?

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 24, 2012

Owner

Is there a reason you have to refire the click event?

Is there a reason you have to refire the click event?

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

We only want this to occur when users click on the toggle-all checkbox right? That's why I am refining this here.

Contributor

boushley replied Feb 24, 2012

We only want this to occur when users click on the toggle-all checkbox right? That's why I am refining this here.

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

And I don't think that Component's have a click event by default.

Contributor

boushley replied Feb 24, 2012

And I don't think that Component's have a click event by default.

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 25, 2012

Owner

I've Ext'ed up this component a bit. Annoyingly it seems ExtJS uses <input type="button"> for checkboxes, so I had to override that.

// Force ExtJS to use a checkbox instead of a button
Ext.define('ss.Checkbox', {
    extend: 'Ext.form.field.Checkbox',
    afterRender: function() {
        this.inputEl.dom.type = 'checkbox';
        this.callParent(arguments);
    }
});

Ext.define('Todo.view.CheckAllBox' , {
    extend: 'ss.Checkbox',
    alias: 'widget.checkAllBox',
    boxLabel: 'Mark all as complete',
    handler: function() {
        console.log('checked', this.getValue() );
    }
});

You then have access to all the Checkbox methods, like getting the value with .getValue() or handle a check with the .handler method.

Owner

sindresorhus replied Feb 25, 2012

I've Ext'ed up this component a bit. Annoyingly it seems ExtJS uses <input type="button"> for checkboxes, so I had to override that.

// Force ExtJS to use a checkbox instead of a button
Ext.define('ss.Checkbox', {
    extend: 'Ext.form.field.Checkbox',
    afterRender: function() {
        this.inputEl.dom.type = 'checkbox';
        this.callParent(arguments);
    }
});

Ext.define('Todo.view.CheckAllBox' , {
    extend: 'ss.Checkbox',
    alias: 'widget.checkAllBox',
    boxLabel: 'Mark all as complete',
    handler: function() {
        console.log('checked', this.getValue() );
    }
});

You then have access to all the Checkbox methods, like getting the value with .getValue() or handle a check with the .handler method.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 24, 2012

Owner

If I loaded the app, started adding todos, and then checked a todo, the clearButton did not appear. However if I refreshed the app with a todo checked, it loaded the clearButton.

Owner

sindresorhus commented Feb 24, 2012

If I loaded the app, started adding todos, and then checked a todo, the clearButton did not appear. However if I refreshed the app with a todo checked, it loaded the clearButton.

Updated the TaskList to handle input clicks better.
Now once you double click on the task you can begin editing, without the list redrawing.
@boushley

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 24, 2012

Contributor

That scenario (clean app, add todos, check todo) around the clearButton seems to be working for me now. Can you try it after my recent commits, and see if it is still happening.

Contributor

boushley commented Feb 24, 2012

That scenario (clean app, add todos, check todo) around the clearButton seems to be working for me now. Can you try it after my recent commits, and see if it is still happening.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 25, 2012

Owner

This could be simplified, by extending Ext.Button instead:

Ext.define('Todo.view.TaskCompleteButton' , {
    extend: 'Ext.Button',
    alias: 'widget.completeButton',
    tpl: new Ext.XTemplate('<a id="clear-completed">{text}</a>', {compiled: true}),
    setText: function (text) {
        this.update({text:text});
    }
});

This could be simplified, by extending Ext.Button instead:

Ext.define('Todo.view.TaskCompleteButton' , {
    extend: 'Ext.Button',
    alias: 'widget.completeButton',
    tpl: new Ext.XTemplate('<a id="clear-completed">{text}</a>', {compiled: true}),
    setText: function (text) {
        this.update({text:text});
    }
});

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Feb 25, 2012

Contributor

You are clearly a better ext.js developer than I :) I thought Ext.Button's always used a tag. I'll include this, thanks.

Contributor

boushley replied Feb 25, 2012

You are clearly a better ext.js developer than I :) I thought Ext.Button's always used a tag. I'll include this, thanks.

boushley added some commits Feb 25, 2012

Added editing on double click.
The editing works as long as you hit enter once you're done. I was having some trouble getting the blur event to take hold. I think the TaskList might need to be broken up to have a component rendering each item.

Also added the remove functionality, however there is a js error that occurs when doing this :/ not sure what causes it, but it works...
@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Feb 26, 2012

Owner

That scenario (clean app, add todos, check todo) around the clearButton seems to be working for me now. Can you try it after my recent commits, and see if it is still happening.

Fixed :)

Owner

sindresorhus commented Feb 26, 2012

That scenario (clean app, add todos, check todo) around the clearButton seems to be working for me now. Can you try it after my recent commits, and see if it is still happening.

Fixed :)

@addyosmani

This comment has been minimized.

Show comment Hide comment
@addyosmani

addyosmani Mar 1, 2012

Owner

Would you guys still like someone from ExtJS to review this? If so, I'll reach out and see what we can do.

Owner

addyosmani commented Mar 1, 2012

Would you guys still like someone from ExtJS to review this? If so, I'll reach out and see what we can do.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Mar 1, 2012

Owner

It's @boushley' work, but in my opinion, yes, I'd think it would be a good idea to do this on all the apps. If we're going help devs choose between frameworks, we should at least show the best from and how to use each framework.

Owner

sindresorhus commented Mar 1, 2012

It's @boushley' work, but in my opinion, yes, I'd think it would be a good idea to do this on all the apps. If we're going help devs choose between frameworks, we should at least show the best from and how to use each framework.

@boushley

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Mar 6, 2012

Contributor

So I've eliminated most of the problems, but there are still a few remaining:

  • The focus outlines are still in place
  • Edit does not end on blur (I'm having some trouble getting this one working, it wasn't supported previously, so I have to do a bit more reading, feel free to help out if you can come up with something @sindresorhus )
  • onStoreDataChanged still does not use a template for the strings it creates.
  • Still no consistency on new Ext.XTemplate vs Ext.create('Ext.XTemplate' (not sure which standard to choose)
Contributor

boushley commented Mar 6, 2012

So I've eliminated most of the problems, but there are still a few remaining:

  • The focus outlines are still in place
  • Edit does not end on blur (I'm having some trouble getting this one working, it wasn't supported previously, so I have to do a bit more reading, feel free to help out if you can come up with something @sindresorhus )
  • onStoreDataChanged still does not use a template for the strings it creates.
  • Still no consistency on new Ext.XTemplate vs Ext.create('Ext.XTemplate' (not sure which standard to choose)
@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Mar 8, 2012

Owner

1 - Outline fix:

:focus {
    outline: none;
}

2 - Just remember that the blur event does not bubble, so even delegation won't work...

4 - Looks like Ext.create is the way to go. Sencha should really update their docs to reflect this. I had to dig pretty deep to figure this out.

Owner

sindresorhus commented Mar 8, 2012

1 - Outline fix:

:focus {
    outline: none;
}

2 - Just remember that the blur event does not bubble, so even delegation won't work...

4 - Looks like Ext.create is the way to go. Sencha should really update their docs to reflect this. I had to dig pretty deep to figure this out.

@addyosmani

This comment has been minimized.

Show comment Hide comment
@addyosmani

addyosmani Mar 19, 2012

Owner

@sindresorhus I've just reviewed @boushley's changes and they look okay to merge. Do you feel 2. above has been addressed appropriately?

It would be great if we could get @edspencer or someone else from Sencha involved in a review of whether this is the best way to advocate using ExtJS for MVC-like apps.

Owner

addyosmani commented Mar 19, 2012

@sindresorhus I've just reviewed @boushley's changes and they look okay to merge. Do you feel 2. above has been addressed appropriately?

It would be great if we could get @edspencer or someone else from Sencha involved in a review of whether this is the best way to advocate using ExtJS for MVC-like apps.

@boushley

This comment has been minimized.

Show comment Hide comment
@boushley

boushley Mar 19, 2012

Contributor

2 has not been addressed. I think we need to redesign the list further, not to use the normal html templates that it has been using. Unfortunately this is over my head as far as extjs goes... and I'm working on ramping up for my new job. So if someone else wants to take a stab at that, please feel free.

Contributor

boushley commented Mar 19, 2012

2 has not been addressed. I think we need to redesign the list further, not to use the normal html templates that it has been using. Unfortunately this is over my head as far as extjs goes... and I'm working on ramping up for my new job. So if someone else wants to take a stab at that, please feel free.

@ettavolt

This comment has been minimized.

Show comment Hide comment
@ettavolt

ettavolt May 5, 2012

From my opinion, the way app is designed - not the best way to utilize what ExtJS has, because it doesn't use existing widgets. If app, that uses bundled widgets, is accepted, I could try to build one.

ettavolt commented May 5, 2012

From my opinion, the way app is designed - not the best way to utilize what ExtJS has, because it doesn't use existing widgets. If app, that uses bundled widgets, is accepted, I could try to build one.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 5, 2012

Owner

@ettavolt We would love some help on this one ;)

We just need to make sure we can implement our template using the builtin widgets. Is that possible?

Owner

sindresorhus commented May 5, 2012

@ettavolt We would love some help on this one ;)

We just need to make sure we can implement our template using the builtin widgets. Is that possible?

@ettavolt

This comment has been minimized.

Show comment Hide comment
@ettavolt

ettavolt May 7, 2012

While page contents (tags/attributes) will differ significantly, it is possible to make it look like template.

ettavolt commented May 7, 2012

While page contents (tags/attributes) will differ significantly, it is possible to make it look like template.

@addyosmani

This comment has been minimized.

Show comment Hide comment
@addyosmani

addyosmani May 7, 2012

Owner

@ettavolt I think we would be open to reviewing an implementation that does make the best use of ExtJS. If you would be willing to put together something which follows our specs as closely as possible (i understand there may be differences) we could review and consider whether to include it over what's been done so far.

@sindresorhus related: I've tried reaching out to the ExtJS camp a few times with little success. I'll see if I can get at least someone well skilled in it in the community to come in as a reviewer for this (just so we're not doing anything too crazy) :)

Owner

addyosmani commented May 7, 2012

@ettavolt I think we would be open to reviewing an implementation that does make the best use of ExtJS. If you would be willing to put together something which follows our specs as closely as possible (i understand there may be differences) we could review and consider whether to include it over what's been done so far.

@sindresorhus related: I've tried reaching out to the ExtJS camp a few times with little success. I'll see if I can get at least someone well skilled in it in the community to come in as a reviewer for this (just so we're not doing anything too crazy) :)

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 7, 2012

Owner

@addyosmani Sounds like a plan.

I've also tried reaching out a couple of times on their IRC a while ago, got no response...

Owner

sindresorhus commented May 7, 2012

@addyosmani Sounds like a plan.

I've also tried reaching out a couple of times on their IRC a while ago, got no response...

@ettavolt

This comment has been minimized.

Show comment Hide comment
@ettavolt

ettavolt May 9, 2012

I've built app with bundled widgets, but it doesn't yet support routing and has completely different look (the default of ext components). Maybe in next week I'll style it up.

ettavolt commented May 9, 2012

I've built app with bundled widgets, but it doesn't yet support routing and has completely different look (the default of ext components). Maybe in next week I'll style it up.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 9, 2012

Owner

@ettavolt Awesome! Thanks for doing this :)

Also, make sure you read the app spec thoroughly ;)

Owner

sindresorhus commented May 9, 2012

@ettavolt Awesome! Thanks for doing this :)

Also, make sure you read the app spec thoroughly ;)

@ettavolt

This comment has been minimized.

Show comment Hide comment
@ettavolt

ettavolt May 10, 2012

Added routes. However, I can't figure out how to push to different branch - git says 403.

Added routes. However, I can't figure out how to push to different branch - git says 403.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 10, 2012

Owner

@ettavolt 403 means you don't have permission. Make sure your trying to push to a published branch on your own fork.

Owner

sindresorhus commented May 10, 2012

@ettavolt 403 means you don't have permission. Make sure your trying to push to a published branch on your own fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment