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

LaserPointerNode: support for accessibleName and helpText #877

Closed
pixelzoom opened this issue Oct 20, 2024 · 6 comments
Closed

LaserPointerNode: support for accessibleName and helpText #877

pixelzoom opened this issue Oct 20, 2024 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

Related to phetsims/models-of-the-hydrogen-atom#67 and phetsims/sun#901 ...

LaserPointerNode currently has problems with support of ParallelDOM accessibleName and helpText options. You have to do something like this, and both options are ignored; they do not appear in the A11y View:

    const laserPointerNode = new LaserPointerNode( ... {
      ...
      buttonAccessibleName: 'This is the acessible name.'
      helpText: 'This is the help text.'
    } );

LaserPointerNode has many options related to its button. Instead of using nested options buttonOptions, there are individual options at the top-level of LaserPointerNodeOptions. The first task will be to convert this to nested options.

The second task is to fix the support for accessible name. This diff seems to fix the problem:

- labelContent: options.buttonAccessibleName,
- labelTagName: 'label',
+ accessibleName: options.buttonAccessibleName,
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 24, 2024

@jessegreenberg I'd be happy to handle this one. But which of these APIs (or something else?) do you think is most appropriate?

// API 1: accessibleName and helpText are specified via nested options to LaserPointerNode's button.
const laserPointerNode = new LaserPointerNode( ..., {
  buttonOptions: {
    accessibleName: ...
    helpText: ...
  }
} );

// API 2: accessibleName and helpText are top-level options, and LaserPointerNode is responsible for propagating them to its button.
const laserPointerNode = new LaserPointerNode( ..., {
  accessibleName: ...
  helpText: ...
} );

@pixelzoom pixelzoom self-assigned this Oct 24, 2024
@pixelzoom
Copy link
Contributor Author

I also see LaserPointerNodeOptions has buttonDescriptionContent?: string. Can that be replaced by helpText?

pixelzoom added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Oct 25, 2024
pixelzoom added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Oct 29, 2024
@pixelzoom
Copy link
Contributor Author

@jessegreenberg and I discussed the 2 APIs in #877 (comment), and we think that API 2 might be the better general choice for PhET UI components. My certainty is ~75%.

In the meantime, I made a couple of small fixes and improvements in 1b893ec, so that these features work in MOTHA. The only other sim affected was Rutherford Scattering, and it does not have a complete description implementation.

@pixelzoom pixelzoom removed their assignment Oct 29, 2024
jessegreenberg added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Oct 30, 2024
jessegreenberg added a commit to phetsims/rutherford-scattering that referenced this issue Oct 30, 2024
@jessegreenberg
Copy link
Contributor

In the above commits I replaced buttonAccessibleName and buttonHelpText with the accessibleName and helpText implementation. Next Ill take a look at converting the converting the other button options to nested options as part of this issue.

jessegreenberg added a commit to phetsims/beers-law-lab that referenced this issue Oct 30, 2024
jessegreenberg added a commit to phetsims/bending-light that referenced this issue Oct 30, 2024
jessegreenberg added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Oct 30, 2024
jessegreenberg added a commit to phetsims/quantum-measurement that referenced this issue Oct 30, 2024
jessegreenberg added a commit to phetsims/rutherford-scattering that referenced this issue Oct 30, 2024
jessegreenberg added a commit to phetsims/wave-interference that referenced this issue Oct 30, 2024
@jessegreenberg
Copy link
Contributor

The above commits use nested options for the button of LaserPointerNode. @pixelzoom can you please review 1c8b15f? In particular using combineOptions and type ButtonOptions.

@pixelzoom
Copy link
Contributor Author

This looks great, including ButtonOptions and the use of combineOptions. Thanks for handling this. 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