-
Notifications
You must be signed in to change notification settings - Fork 948
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 a Style widget #219
Add a Style widget #219
Conversation
6afb8f1
to
0086bbe
Compare
style: {deserialize: unpack_models}, | ||
}, WidgetModel.serializers), | ||
|
||
setStyle: function(style, oldStyle) { |
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.
Do we need to undo the styles applied by the oldStyle when a new one is set?
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 probably should try to do this, but it gets tricky. I'll try doing this and ping back.
e1f1626
to
0ed994c
Compare
Action shot: @SylvainCorlay ready for review |
@SylvainCorlay a couple of questions:
I'm sure you see the design of this is a little different than we talked about. This is basically a large subset of CSS keys, all that I could find which were cross platform. I think it makes sense to do this, instead of shooting ourselves in the foot by trying to abstract CSS into simpler keys. |
IKR, we really need to move away from Phantom. I'll make that a priority for 6.0 |
a1e42af
to
71e2819
Compare
@@ -160,7 +160,7 @@ define([ | |||
|
|||
// Trigger the displayed event of the child view. | |||
that.displayed.then(function() { | |||
view.trigger('displayed'); | |||
view.trigger('displayed', that); |
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.
Why you need to add this argument?
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.
Widgets and WidgetModels don't have a notion of parent, this is as-designed. However, DOM nodes have a notion of parent and children yet DOMWidgetViews do not. By adding that, I'm adding the idea of a parent view to the DOMWidgetView. I though this made sense, because the alternative would be to make the StyleView have an invisible el which gets attached to the DOM, and then use that to get the parent. All of this is because the Style applies to the parent.
71e2819
to
c43fb9d
Compare
What do you mean regarding the Union? |
I was wondering if, for things like |
also fix a bug in deserialization on construction
c43fb9d
to
3662544
Compare
I talked to @SylvainCorlay before he left for the airport and he said it was okay to merge this. |
Which, when displayed, is capable of changing style attributes on the parent.
TODO