Skip to content
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 checkbox button #528

Merged
merged 7 commits into from
Jan 13, 2020
Merged

Add checkbox button #528

merged 7 commits into from
Jan 13, 2020

Conversation

GuillaumeFavelier
Copy link
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
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
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
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
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
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
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.

None yet

3 participants