Skip to content

Default values for widget model attributes #304

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

Merged
merged 16 commits into from
Jan 12, 2016

Conversation

SylvainCorlay
Copy link
Member

@jdfreder @parente here is the approach I am proposing for specifying default values for widget attributes on the javascript side.

I started with the base widget classes and the _Bool widgets.

TODO:

  • figure out what to do with the instance of layout widget.
  • check commit regarding buffered state diff.
  • some widgets have the same models and view types, but different default values. Is it worth inheriting and specifying a custom model for these cases (range sliders vs sliders)

This is an API change from the perspective of custom widget authors who need custom models.

@jdfreder jdfreder added this to the 5.0 milestone Jan 8, 2016
@jdfreder
Copy link
Contributor

jdfreder commented Jan 8, 2016

From the sample you have here, I think this is great.

@SylvainCorlay
Copy link
Member Author

Test failures seem unrelated.

});

var CheckboxModel = BoolModel.extend({
defaults: _.extend(BoolModel.prototype.defaults, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line that is causing the tests to fail. You need to import underscore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank the new karma tests ;)

@jdfreder
Copy link
Contributor

jdfreder commented Jan 8, 2016

Test failures seem unrelated.

They are related, missing import, see above

@SylvainCorlay
Copy link
Member Author

Thanks. I will iterate on this PR to cover the other models.

@SylvainCorlay
Copy link
Member Author

  • The color picker widget has a recently added boolean attribute called short specifying whether it should take a full widget width and have a text input area or just have a picker button and have a short widget width. We could rename this to "concise".

@SylvainCorlay SylvainCorlay force-pushed the default_values branch 6 times, most recently from 5b5cff5 to dcb2d24 Compare January 11, 2016 15:14
@jdfreder
Copy link
Contributor

We could rename this to "concise".

Sure

@parente
Copy link
Contributor

parente commented Jan 11, 2016

Ping @lbustelo

@SylvainCorlay
Copy link
Member Author

We could rename this to "concise".

Sure

Done

@SylvainCorlay
Copy link
Member Author

I have to figure out how to define a default for the instance of layout widget from the js, the default being to instantiate a new one...

@jdfreder
Copy link
Contributor

Great work so far! Looking forward to this one

@SylvainCorlay SylvainCorlay force-pushed the default_values branch 3 times, most recently from 02e636e to 39cbdf3 Compare January 11, 2016 22:29
@SylvainCorlay
Copy link
Member Author

I would love to have your opinion on the buffered_state_diff commit.

@SylvainCorlay
Copy link
Member Author

The issue was that the state from defaults was being added to buffered_state_diff at the time when the base constructor was called. Then the default values were sent back to the backend.

I just changed the code in the following fashion: only buffer state diff after the initial state has been set.

var DOMWidgetModel = WidgetModel.extend({}, {
var DOMWidgetModel = WidgetModel.extend({
defaults: _.extend({}, WidgetModel.prototype.defaults, {
layout: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to be null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I don't know. We don't allow None for the layout widget.
In fact, even when building it from JavaScript, we should instantiate a layout widget. Not sure what is the best way to do it. That is why I used undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay for now, note to self: test edge cases of this.

@jdfreder
Copy link
Contributor

@SylvainCorlay one small comment

jdfreder added a commit that referenced this pull request Jan 12, 2016
Default values for widget model attributes
@jdfreder jdfreder merged commit 29871a1 into jupyter-widgets:master Jan 12, 2016
@SylvainCorlay SylvainCorlay deleted the default_values branch January 12, 2016 18:53
@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature request resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants