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

optimize margin around ComboBoxButton's arrow button #453

Closed
pixelzoom opened this issue Jan 17, 2019 · 7 comments
Closed

optimize margin around ComboBoxButton's arrow button #453

pixelzoom opened this issue Jan 17, 2019 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

Discussed with @ariel-phet.

ComboBoxButton's xMargin and yMargin are currently applied to both the item and the arrow parts of the button. But as xMargin increases, the margin around the arrow becomes unnecessarily large. In general, we want to keep the arrow part of the button looking roughly square. So...

I'll investigate computing the margin around the arrow so it looks square. If that's not possible, I'll add an arrowXMargin. I'd most definitely prefer the former.

@pixelzoom pixelzoom self-assigned this Jan 17, 2019
pixelzoom added a commit to phetsims/balancing-chemical-equations that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/beers-law-lab that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/unit-rates that referenced this issue Jan 17, 2019
pixelzoom added a commit that referenced this issue Jan 17, 2019
@pixelzoom
Copy link
Contributor Author

ComboBoxButton is now divided into 2 "areas" with a vertical separator between them.

The "item area" is to the left of the separator and displays the selected item. It's margin is determined by options xMargin and yMargin.

The "arrow area" is to the right of the separator and displays an up or down arrow, depending on the value of options.arrowDirection. This area is a square whose side length is maxItemLength + 2 * options.yMarin. The arrow is 0.35 * the side length (value was chosen by consulting with @ariel-phet).

This provides a uniform "look" for the button's arrow without requiring additional options. With the change and the changes in #430, many margin-related options have been reduced to xMargin and yMargin for ComboBox and its subcomponents.

Example in BLL:

screenshot_972

Examples in Gas Properties:

screenshot_974

Examples in scenery-phet demo:

screenshot_973

@pixelzoom
Copy link
Contributor Author

@ariel-phet please assign someone to review.

@pixelzoom
Copy link
Contributor Author

@ariel-phet ping. I'd like to get the ComboBox work wrapped up.

@Denz1994
Copy link
Contributor

Denz1994 commented Jan 24, 2019

@pixelzoom These options look great. Just one note and close if this is a non-issue.

Setting xMargin:0 and yMargin:0 causes the edges ComboBoxListItemNode.highlightRectangle to extend beyond the edges of the ComboListBox if the ComboListBox has a cornerRadius > 0.

Perhaps this is a good time to pass ComboBox.cornerRadius to ComboBoxListItemNode.highlightRectangle via const listItemNode = new ComboListItemNode(...) in ComboBoxListBox.js

Here is an example in Gas-Properties:
image

Also in MAS:
image

Assigning @pixelzoom for input.

@Denz1994 Denz1994 reopened this Jan 24, 2019
@Denz1994
Copy link
Contributor

Accidently closed.

@Denz1994 Denz1994 assigned pixelzoom and unassigned Denz1994 Jan 24, 2019
pixelzoom added a commit that referenced this issue Jan 24, 2019
pixelzoom added a commit that referenced this issue Jan 24, 2019
Signed-off-by: Chris Malley <[email protected]>
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 24, 2019

Thanks @Denz1994, good suggestion about corner radius.

Changes in the above commits:

  • I added highlightCornerRadius to ComboBoxListItemNode, and the same value is used for the button, listbox and item highlights in the list. I cleared this design change with @ariel-phet, he gave it a thumbs up.

  • This made me realize that xMargin and yMargin must be > 0, so I added assertions. Otherwise the highlights will overlap the edges of the listbox regardless of the cornerRadius. And since the highlight overlap is 1/2 the value of xMargin and yMargin, having zero margins makes no sense.

  • This also made me realize that ComboBoxListItemNode should have responsibility for its own highlighting. So I moved the relevant input listener into ComboBoxListItemNode. This cleaned up the API quite a bit.

That said... There are undoubtedly ways to make ComboBox look bad. For example xMargin: 1, yMargin: 1, listLineWidth: 10 will look pretty awful. But rather than try to detect every case that might look bad, the designer and programmer need to take a certain amount of responsibility for looking at what they specified and evaluating whether it looks "good".

Back to you for review.

@pixelzoom pixelzoom assigned Denz1994 and unassigned pixelzoom Jan 24, 2019
@Denz1994
Copy link
Contributor

This is a definite improvement and I agree with the below comment:

the designer and programmer need to take a certain amount of responsibility for looking at what they specified and evaluating whether it looks "good".

Closing this one.

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