refactor: Make focusable elements responsible for scrolling themselves into bounds.#9288
refactor: Make focusable elements responsible for scrolling themselves into bounds.#9288
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks! I think it makes a lot of sense to approach it this way. I worry a little about it being easy to forget to auto-scroll in onNodeFocus implementations, but I don't have a better suggestion on the approach.
Is it possible to add tests for the different components to verify that they auto-scroll correctly onto the screen? I realize that might be a lot to test, but if it's important to work then we probably should ensure that it stays working, if possible.
|
Added browser tests to ensure that focusing elements moves the workspace such that the element's bounds are visible. |
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @gonfunko! Approving to unblock, but had a few follow-ups: PTAL.
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Really nice tests, and I think the latest solution is much cleaner.
…s into bounds. (#9288) * refactor: Make focusable elements responsible for scrolling themselves into bounds. * chore: Add tests for scrolling focused elements into view. * fix: Removed inadvertent `.only`. * fix: Scroll parent block of connections into bounds on focus.
2 participants
The basics
The details
Resolves
Fixes #9212
Proposed Changes
This PR updates
LineCursorto no longer attempt to scroll elements into view when they are focused. Instead, this responsibility is delegated to the elements themselves in theonNodeFocus()callback. This simplifies the implementation ofLineCursorby not tying it to specific kinds of elements, and means that additional focusable elements (whether first or third party) can adopt this behavior.