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

KeyboardDragListener does not support dragBoundsProperty. #1307

Closed
pixelzoom opened this issue Oct 25, 2021 · 8 comments
Closed

KeyboardDragListener does not support dragBoundsProperty. #1307

pixelzoom opened this issue Oct 25, 2021 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

Geometric Optics has dynamic drag bounds. If you resize the browser window, it makes use of the available space.

While adding alt input support in phetsims/geometric-optics#235, I discovered that KeyboardDragListener does not have the same dragBounds API as DragListener. DragListener supports dynamic drag bounds via option {Property.<Bounds2>} dragBoundsProperty. KeyboardDragListener has option {Bounds2} dragBounds and set dragBounds, so does not support a Property.

I can work around this difference for Geometric Optics by adding my own listener to my dragBoundsProperty and calling KeyboardDragListener's setter when it changes. But that should be KeyboardDragListener's responsibility; KeyboardDragListener and DragListener should have the same API for dragBounds.

@zepumph
Copy link
Member

zepumph commented Oct 25, 2021

Thanks! I think there is an open issue for this somewhere. Also please note that this falls under work to be done as part of #1298 (tagging it here).

I couldn't find another issue in https://github.com/phetsims/scenery/issues?q=is%3Aissue+is%3Aopen+KeyboardDragListener

I know this is something that @jessegreenberg and I have discussed in the past, but just didn't get enough priority (until someone wanted to use it). I was just in here recently discussing this for Friction, and I reviewed the discrepancy between DragListener.dragBoundsProperty and KeyboardDragListener's field. It shouldn't be too hard to add.

@pixelzoom
Copy link
Contributor Author

I'm running into this again in Geometric Optics, where the rulers have dynamic drag bounds. Adding to Developer Meeting project board.

@zepumph
Copy link
Member

zepumph commented Dec 16, 2021

I can tackle this now.

@zepumph
Copy link
Member

zepumph commented Dec 16, 2021

@pixelzoom are you adding to dev meeting to get traction on this issue, or would you like to discuss something in particular?

@pixelzoom
Copy link
Contributor Author

I was adding to get traction. Feel free to remove it from the project board since you're tackling it now.

@pixelzoom
Copy link
Contributor Author

Also note that I've been mentioning this issue in TODO comment is my code, to identify sites that will need to be fixed up.

zepumph added a commit to phetsims/balloons-and-static-electricity that referenced this issue Dec 16, 2021
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Dec 16, 2021
zepumph added a commit to phetsims/fourier-making-waves that referenced this issue Dec 16, 2021
zepumph added a commit to phetsims/geometric-optics that referenced this issue Dec 16, 2021
@zepumph
Copy link
Member

zepumph commented Dec 16, 2021

Committed above + usages. This includes cleaning up the TODOs in GO. @pixelzoom does this look right to you in GO and any other usages you are aware of (I changed one in Fourier as well)?

@pixelzoom
Copy link
Contributor Author

Looks great, thanks! 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