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

Right-align contents of A11yButtonsHBox #747

Closed
arouinfar opened this issue Sep 17, 2021 · 6 comments
Closed

Right-align contents of A11yButtonsHBox #747

arouinfar opened this issue Sep 17, 2021 · 6 comments

Comments

@arouinfar
Copy link

A sim can currently have up to three buttons grouped next to the PhET logo:
image

With PhET-iO customization, one or more of these buttons can be hidden, leaving behind a gap between the buttons and the PhET-iO logo. It already appears that the HBox is utilizing dynamic layout, since the keyboard button shifts over when the audio button is hidden:
image

It seems like the buttons are left-aligned within their box, and as icons are hidden, the remaining contents scoot to the left. It would be preferable to instead right-align the contents of the HBox, so that the padding between the rightmost button and the PhET-iO logo is consistent.

Here are some mockups of what it would look like if one or more of the buttons is hidden:
image

This doesn't really feel like a PhET-iO specific issue, though it was motivated by PhET-iO design, so I'll pass this off to the responsible dev, @zepumph.

@zepumph
Copy link
Member

zepumph commented Sep 20, 2021

@jessegreenberg, can you help with this, I know you have done a fair bit of this layout.

I tried this patch, and thought it would work because the layout is set from the right of the node (

this.a11yButtonsHBox.right = this.phetButton.left - PHET_BUTTON_LEFT_MARGIN;
)

Index: js/A11yButtonsHBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/A11yButtonsHBox.js b/js/A11yButtonsHBox.js
--- a/js/A11yButtonsHBox.js	(revision 7f9fce34497d46ea70e59fce32698a952fb79c72)
+++ b/js/A11yButtonsHBox.js	(date 1632152710280)
@@ -26,7 +26,8 @@
   constructor( sim, backgroundColorProperty, tandem, options ) {
 
     options = merge( {
-      align: 'center',
+      align: 'right',
+      resize: true, // maybe?
       spacing: 6
     }, options );
 

@jessegreenberg, what do you think?

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Sep 20, 2021
@jessegreenberg
Copy link
Contributor

I think re-calling this part of NavigationBar.layout is what would be needed for this.

joist/js/NavigationBar.js

Lines 339 to 342 in 33aab76

if ( this.a11yButtonsHBox.getChildrenCount() > 0 ) {
this.a11yButtonsHBox.right = this.phetButton.left - PHET_BUTTON_LEFT_MARGIN;
}

right alignment is only for vertical LayoutBoxes and resize: true should be enabled by default, though using that is probably important so the A11yButtonsHBox does have the correct bounds when setting its right.

@zepumph
Copy link
Member

zepumph commented Sep 20, 2021

But the a11y buttons is an HBox, and thus, a LayoutBox, so I thought we would be getting this for free from excludeInvisibleChildrenFromBounds. This should call a relayout on the hbox each time any child's visibility is changed. I'll keep investigating

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Sep 20, 2021

That's true, but changing layout of HBox children won't automatically reposition the HBox itself. On master, if you toggle general.view.navigationBar.audioToggleButton.visibleProperty and then resize the sim the A11yButtonsHbox.right is set correctly.

@zepumph
Copy link
Member

zepumph commented Sep 20, 2021

With this code in NavigationBar.js I was able to confirm that the HBox was re-laying out, but that it didn't respect the right dimension of it. Thanks for explaining.

    const x = Rectangle.bounds( this.a11yButtonsHBox.bounds, { stroke: 'red' } );

    this.a11yButtonsHBox.boundsProperty.link( bounds => {
      x.rectBounds = bounds;
    } );

    this.barContents.addChild( x );

I also didn't want to deal with calling the whole layout function, which would require dimensions and scale parameters. Instead I factored out just the re-layout code that I needed for this task.

What do you think @jessegreenberg?

@jessegreenberg
Copy link
Contributor

Your change was great, thanks. Note that master now uses ManualConstraint for dynamic layout so this can be closed.

joist/js/NavigationBar.ts

Lines 305 to 311 in 74f3739

ManualConstraint.create( this.barContents, [ phetButton, this.a11yButtonsHBox ], ( phetButtonProxy, a11yButtonsHBoxProxy ) => {
a11yButtonsHBoxProxy.right = phetButtonProxy.left - PHET_BUTTON_LEFT_MARGIN;
// The icon is vertically adjusted in KeyboardHelpButton, so that the centers can be aligned here
a11yButtonsHBoxProxy.centerY = phetButtonProxy.centerY;
} );

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