Skip to content

Add checkbox button#528

Merged
GuillaumeFavelier merged 7 commits intomasterfrom
add_button_widget
Jan 13, 2020
Merged

Add checkbox button#528
GuillaumeFavelier merged 7 commits intomasterfrom
add_button_widget

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

This PR adds basic support for checkbox button. The Animate label below is not part of the button but is given by add_text() instead.

To illustrate (inspired by Orbiting):

output

Source code: https://gist.github.com/GuillaumeFavelier/1bfa7ad2160d86b0814ed890dd7f27bc

I think this demo could be improved with floors or background image.

@GuillaumeFavelier GuillaumeFavelier added the feature-request Please add this cool feature! label Jan 10, 2020
@GuillaumeFavelier GuillaumeFavelier self-assigned this Jan 10, 2020
Copy link
Copy Markdown
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Excellent, I agree with the implementation. We will want to add tests for this once we’ve figured out how to get interactive callbacks working.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

I can add it to the tests at least for coverage then

func = lambda value: value # Does nothing
p.add_mesh(mesh)
p.add_checkbox_button_widget(callback=func)
p.clear_button_widgets()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's remove this line. The clear method should be called on close and if not, it will segfault resulting in a failed test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed it but I think the same is done in all the other tests. In this case we should correct this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah yep, i totally missed that all the other tests are doing that. we should fix those too

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

I'll go ahead and merge this then I'll open a follow-up PR to fix the tests as suggested in #528 (comment).

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

Labels

feature-request Please add this cool feature!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants