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

pointer areas sign-off #192

Closed
pixelzoom opened this issue Aug 20, 2020 · 14 comments
Closed

pointer areas sign-off #192

pixelzoom opened this issue Aug 20, 2020 · 14 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 20, 2020

This is listed as a "standard GitHub issue" in the code-review checklist. The lead designer must sign off on pointer areas.

@pixelzoom pixelzoom self-assigned this Aug 20, 2020
@pixelzoom pixelzoom changed the title review pointer areas pointer areas sign-off Aug 20, 2020
@pixelzoom
Copy link
Contributor Author

@amanda-phet this is ready for review. The screenshot below shows pointer areas for the Lab screen, Intro screen is identical. Not shown are the Proportions and Pedigree graphs.

Note that the checkboxes in the Population panel do not have uniform width. I'll make them uniform if you want. But unlike radio buttons, there's no support in common code. And imo, uniform width is not desirable for checkboxes or radio buttons.

screenshot_498

@pixelzoom
Copy link
Contributor Author

@ariel-phet and I discussed the Population panel. I'll make all of the checkboxes the same width. It'll probably work out better for focus traversal.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 25, 2020

Population checkbox touch areas have been expanded, see screenshot below. Proportions checkbox has not - there's only one, so nothing to expand to match. I'd have to do something evil, like add an invisible horizontal strut, to make that one fill the control panel. Pedigree checkboxes are definitely not appropriate for pointer areas that fill the panel.

@ariel-phet please review in master.

screenshot_521

@pixelzoom
Copy link
Contributor Author

Hold off on reviewing. Making the Population checkboxes all have the same size created a new problem, #207. So I'm going to have to come up with a different way of doing this.

@pixelzoom
Copy link
Contributor Author

Fixed in the above commit. You may resume reviewing.

@pixelzoom
Copy link
Contributor Author

@amanda-phet it looks like @ariel-phet has gotten busy, so assigning to you too. Whichever one of you gets to this first...

@amanda-phet
Copy link
Contributor

I think it all looks fine.

I can't see the pointer areas for the Fur, Ears, and Teeth checkboxes on the Proportions screen. When I try to discern them with my cursor, they don't seem as roomy as the other checkboxes. It seems like I need to be right on the text or box, and not in a roomy rectangle around the text and box.

@pixelzoom any way to confirm what those areas are, or expand them slightly?

@pixelzoom
Copy link
Contributor Author

Good catch, addressed in the above commit. I've used the same dilation that's used for other checkboxes. See screenshot below. Back to @amanda-phet for review.

screenshot_527

@pixelzoom pixelzoom removed their assignment Aug 27, 2020
pixelzoom added a commit that referenced this issue Aug 27, 2020
@amanda-phet
Copy link
Contributor

Looks good. Thanks!

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 28, 2020

Reopening.

After testing on his phone, @ariel-phet has some requests. Increase touchArea for these UI components:

Definitely:

  • fast-forward button (6 looked good)
  • y-axis zoom buttons (asymmetric, common-code)

Bonus:

  • Generation arrow buttons below Population graph
  • Generation arrow buttons below Proportions graph (NumberSpinner)

We're not going to do these for the prototype RC.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 28, 2020

Here's a patch for the fast-forward button. We thought that 6 looked like a good dilation. But if we want the touchArea to be the same as the play button (and to be computed, instead of a magic number), then the correct value is 4. I'll discuss this with @ariel-phet later.

patch
Index: js/common/view/NaturalSelectionTimeControlNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/NaturalSelectionTimeControlNode.js	(revision 23f61b80837844e5347b912447f78e8ec6439491)
+++ js/common/view/NaturalSelectionTimeControlNode.js	(date 1598644626382)
@@ -19,6 +19,10 @@
 import naturalSelection from '../../naturalSelection.js';
 import FastForwardButton from './FastForwardButton.js';
 
+// constants
+const PLAY_BUTTON_RADIUS = 20;
+const FAST_FORWARD_BUTTON_RADIUS = 16;
+
 class NaturalSelectionTimeControlNode extends HBox {
 
   /**
@@ -40,12 +44,14 @@
     }, options );
 
     const playPauseButton = new PlayPauseButton( isPlayingProperty, {
-      radius: 20,
+      radius: PLAY_BUTTON_RADIUS,
       listener: () => fastForwardButton.interruptSubtreeInput(),
       tandem: options.tandem.createTandem( 'playPauseButton' )
     } );
 
     const fastForwardButton = new FastForwardButton( timeSpeedProperty, {
+      radius: FAST_FORWARD_BUTTON_RADIUS,
+      touchAreaDilation: PLAY_BUTTON_RADIUS - FAST_FORWARD_BUTTON_RADIUS,
       tandem: options.tandem.createTandem( 'fastForwardButton' )
     } );

touchAreaDilation: 6
screenshot_531

touchAreaDilation: 4
screenshot_530

@pixelzoom
Copy link
Contributor Author

I've noticed that for the sim-specific dialogs, the close button feels a little finicky for me on iPad. Since there are no UI components in these dialogs, there's no reason that we can't make their close buttons have generous pointer areas. So I've done that, they are all the same size, and they look like this:

screenshot_537

screenshot_536

screenshot_535

@pixelzoom
Copy link
Contributor Author

All requests have been completed. Most of them are shown in the first screenshot below. Generations below the Proportions graph is shown in the second screenshot. See also the screenshots in the previous comment related to Dialog close buttons.

@ariel-phet please verify in master, and feel free to close.

screenshot_538

screenshot_539

@ariel-phet
Copy link

Played on my phone and the few previous problem areas were much more usable. Closing.

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