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

HSlider: use pixels instead of percentages for touchArea default #254

Closed
pixelzoom opened this issue Aug 29, 2016 · 11 comments
Closed

HSlider: use pixels instead of percentages for touchArea default #254

pixelzoom opened this issue Aug 29, 2016 · 11 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 29, 2016

In light of #251 (comment), HSlider's defaults for touchArea should be changed to pixels instead of percentages. This will certainly affect many clients.

@pixelzoom pixelzoom self-assigned this Aug 29, 2016
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 29, 2016

The bit of code to be changed is at line 162:

    // Touch area for the default thumb. If a custom thumb is provided, the client is responsible for its touchArea.
    if ( !options.thumb ) {

      if ( options.thumbTouchAreaXDilation === null ) {
        options.thumbTouchAreaXDilation = 0.5 * thumb.width;
      }

      if ( options.thumbTouchAreaYDilation === null ) {
        options.thumbTouchAreaYDilation = 0.25 * thumb.height;
      }

      // touch area dilated along X and Y directions
      thumb.touchArea = thumb.bounds.dilatedXY( options.thumbTouchAreaXDilation, options.thumbTouchAreaYDilation );
    }

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 29, 2016

Proposal: Change the default touch dilations to be 0.5 and 0.25 of the default thumb width and height respectively. I.e.,

     thumbSize: new Dimension2( 22, 45 ),
     ...
     thumbTouchAreaXDilation: 11,
     thumbTouchAreaYDilation: 11,

This will preserve the existing behavior for clients that use the default thumb size. And I can identify clients whose behavior may change by searching for "thumbSize:".

@samreid Do you forsee any problems with this plan?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 29, 2016

45 occurrences of "thumbSize:" in 25 sims. This will be a lot of work.

@samreid
Copy link
Member

samreid commented Aug 29, 2016

Likewise I see 3 places where thumbNode is passed in to HSlider. If those thumbs have different sizes, the behavior could be a bit different. (Not a reason to invalidate your proposal, but they are cases that should probably be checked after the changes are made).

I don't see any other problems with the plan, but would recommend to test a case or two manually and make sure things are matching up as expected, perhaps by console.logging the touch area before/after to make sure it is preserved.

@samreid samreid removed their assignment Aug 29, 2016
@pixelzoom
Copy link
Contributor Author

@samreid said:

Likewise I see 3 places where thumbNode is passed in to HSlider.

When a custom thumbNode is provided, the client is responsible for its pointer areas, as indicated in the documentation:

55 // Dilation of touchArea for default thumb, ignored for custom thumb.

@samreid
Copy link
Member

samreid commented Aug 29, 2016

I see that now, thanks for showing me. Everything seems good to proceed.

@pixelzoom
Copy link
Contributor Author

I might as well add mouseArea options while I'm at it.

pixelzoom added a commit that referenced this issue Aug 29, 2016
pixelzoom added a commit to phetsims/acid-base-solutions that referenced this issue Aug 29, 2016
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 29, 2016

Skype discussion about how to fix clients:

[8/29/16, 10:43:26 AM] Chris Malley: In #251 (comment), we decided to use pixels instead of percentages as the default values for options related to pointer areas. This decision affects HSlider (#254). For sims that use the default thumb size, I can set the default touchArea values such that the behavior doesn't change. But there are 25 sims that use a custom thumbSize, and I'm not looking forward to spending hours fixing their behavior. Would it be OK if I created issues for these 25 sims, assigned to the primary developer to address?
[8/29/16, 10:44:23 AM] Jesse Greenberg: That sounds fine to me!
[8/29/16, 10:45:47 AM] John Blanco: Works for me too.

Issues have been created for all sims that require adjustment, see list of issues immediately above. These are the sims that create one or more HSliders with the thumbSize option. They will also need to specify thumbTouchAreaXDilation and thumbTouchAreaXDilation.

@pixelzoom
Copy link
Contributor Author

I took care of all the sims that didn't have a clear primary developer (color-vision, etc.) The remaining issues are assigned to their respective primary developer(s).

@pixelzoom
Copy link
Contributor Author

@jbphet selected at random to review.

@pixelzoom pixelzoom assigned jbphet and unassigned pixelzoom Aug 29, 2016
pixelzoom added a commit that referenced this issue Aug 29, 2016
@jbphet
Copy link
Contributor

jbphet commented Aug 30, 2016

The commits all look good. I tested by running the sun demo with showPointerAreas and changing the mouse and touch area dilation amounts in the HSlider options in ComponentsView.js, and everything worked correctly. I also did some testing on States of Matter yesterday and noticed some asymmetry in the touch area, which @pixelzoom addressed in the commit shown just above. Bottom line: Looks good, closing. Thanks for doing this @pixelzoom.

@jbphet jbphet closed this as completed Aug 30, 2016
andrewadare added a commit to phetsims/capacitor-lab-basics that referenced this issue Sep 14, 2016
andrewadare added a commit to phetsims/energy-forms-and-changes that referenced this issue Sep 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants