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

Use VerticalCheckboxGroup instead of custom ISLCCheckboxItem #66

Closed
zepumph opened this issue Jun 3, 2019 · 7 comments
Closed

Use VerticalCheckboxGroup instead of custom ISLCCheckboxItem #66

zepumph opened this issue Jun 3, 2019 · 7 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jun 3, 2019

I'm unsure why there is a custom is solution here, even after looking at #51.

It seems like we should be using VerticalCheckboxGroup, I'll take a look further. From phetsims/gravity-force-lab-basics#134

@zepumph zepumph self-assigned this Jun 3, 2019
@jessegreenberg
Copy link
Contributor

I think the motivation for removing VerticalCheckboxGroup in #51 was because VerticalCheckboxGroup does not currently support disabling a single checkbox in the group.

@zepumph
Copy link
Member Author

zepumph commented Jul 2, 2019

@jessegreenberg in the above commits I added support to VerticalCheckboxGroup to support individual options passed to a single checkbox. From that it was easy enough to pass a custom enabledProperty to the scientific notation checkbox. Please review. Note that this changed the layout of the checkbox panel by a few pixels (not greatly). What do you think about this change?

@jessegreenberg
Copy link
Contributor

Awesome, thank you! What do you think about changing the order of options for the extend call to

_.extend( {}, options.checkboxOptions, item.options, {

so that the group options can be overridden by individual options?

@zepumph
Copy link
Member Author

zepumph commented Jul 2, 2019

Great idea! I also needed to do some cleanup in terms of how we were passing enabledProperty through (mainly for phet-io related problems). @chrisklus was a great help in simplifying. His idea was to just use the showForceValuesProperty as the enabledProperty, so there was no proxy/intermediary needed. @jessegreenberg anything else for this issue?

@terracoda
Copy link
Contributor

terracoda commented Jul 8, 2019

@zepumph and @jessegreenberg, the disabling option has been working visually, but I don't think it has ever worked in the PDOM. In the screenshot you can see that the disabled check box is focusable and checked.
Screen Shot 2019-07-08 at 5 24 14 PM

This checkbox issue is being tracked in phetsims/gravity-force-lab#110.

@zepumph
Copy link
Member Author

zepumph commented Jul 8, 2019

Sounds good. @terracoda see phetsims/sun#517 for the general solution. Anything else for this issue?

@jessegreenberg
Copy link
Contributor

Changes look great, and since the issue @terracoda mentioned is going to be tracked elsewhere I think this can be closed.

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

No branches or pull requests

3 participants