-
Notifications
You must be signed in to change notification settings - Fork 63
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
Improved Olympics: Material Design, better layout/scrolling, etc. #101
Conversation
@@ -30,8 +30,13 @@ foam.CLASS({ | |||
'foam.u2.TableView' | |||
], | |||
|
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.
The package name of this class should be updated to reflect its new home in come.google.foam.demos
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.
Done.
I've cut this apart into a |
@@ -63,6 +63,7 @@ foam.CLASS({ | |||
properties: [ | |||
'data', | |||
'action', | |||
[ 'nodeName', 'button'], |
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.
Keeping this in initE is actually the preferred style.
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.
Sorry, what? When did that happen?
More importantly, why? Isn't a property you can override more flexible?
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.
Happened one day in a design meeting.
I think both are equally flexibly, when you extend an element you're probably going to write a custom initE() so you can just set the nodeName there.
I personally like the style of initE() more or less completely defining the element. It also keeps it more consistent when nesting elements. If you have a big block of DSL, you get used to configuring elements though DSL, it becomes an impedance mis-match to configure your element through properties but configure all element you create in initE() through DSL.
That was one of my major pain points with foam U1, was all half the time you had to remember which things to do as properties, which as toHTML, which as toInnerHTML. In U2 its all in initE()
It's why .setNodeName() was added to the DSL.
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.
Done.
lgtm |
this.data.select(foam.u2.TableBodySink.create({ properties_: this.properties_ }, this)).then( | ||
function(a) { this.body = a.body; }.bind(this)); | ||
var dao = this.data; | ||
if ( this.sortOrder ) { |
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.
I feel like the more FOAM way of doing this, would be to make a property called orderedDAO, which is an expression of dao and sortOrder.
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.
Ignore this for now. If my DAOSlot work pans out we'll be replacing this anyways.
Moves the demo to src/com/google/foam/demos/olympics to be with the others. You need to run `bower update` in the olympics directory before it will work, and you need to use its custom HTML page: polymer.html. The Polymer-powered version is separate from the original, to help demonstrate the changes necessary to add Polymer to an existing app.
No description provided.