-
Notifications
You must be signed in to change notification settings - Fork 66
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
Initial refactor of todo-mvc-kitchensink #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of comments
todo-mvc-kitchensink/package.json
Outdated
"@dojo/widgets": "^2.0.0-alpha.19", | ||
"@dojo/widget-core": "2.0.0-alpha.25", | ||
"@dojo/i18n": "2.0.0-alpha.6", | ||
"@webcomponents/custom-elements": "^1.0.0-alpha.3", | ||
"maquette": ">=2.3.7 <=2.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you bump this to 2.4.3
please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped!
@@ -12,11 +11,19 @@ interface FormInputEvent extends KeyboardEvent { | |||
target: HTMLInputElement; | |||
} | |||
|
|||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted
onkeyup, | ||
onblur, | ||
oninput, | ||
classes: this.classes(styles.base).get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI you can drop .get()
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all references to .get()!
w(Label, <any> { | ||
theme, | ||
label, | ||
onDoubleClick: todoEdit.bind(this), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't need to bind her anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... if I don't bind it to this
wont it get bound to the widget it is on? In this case I want it to run on the parent widget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this is going to be deleted once i steal your magic from todo-mvc ☠️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we bind functions magically in WidgetBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are automatically bound to the enclosing widget for w
(it wouldn't make sense to bind it to the widget itself).
https://github.com/dojo/widget-core/blob/master/tests/unit/WidgetBase.ts#L162
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you should be able to remove all bind(this)
's in render
functions
onInput: this.onInput, | ||
theme | ||
}), | ||
v('div', {}, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have no properties, children can be the second arg (just FYI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed empty {}
where i found them
} | ||
|
||
return v('ul', { | ||
classes: this.classes(...classList).get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do something like this now also
this.classes(
styles.todoItemList,
todos.length === 0 ? styles.empty : null,
activeView === 'cards' ? styles.cardList : null
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That. Is. Great. Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few places that this can be cleaned up but that will come in follow pull requests.
core/request