Skip to content

Conversation

@gonfunko
Copy link
Contributor

This PR fixes #296 by moving the left/right/up/down arrow key actions out of navigation_controller.ts and into their own class.

@gonfunko gonfunko requested a review from a team as a code owner March 10, 2025 19:39
@gonfunko gonfunko requested review from cpcallen and removed request for a team March 10, 2025 19:39
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: hold off on merging until Ben's current focus changes land.

Copy link
Collaborator

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my remarks on #274—see also this lengthy discussion about cursor actions—I think the actions should be named after the cursor keys they are bound to.

(And we should in due course refactor the cursors so that their APIs are also directional, with each cursor being responsible for mapping from arrow direction to AST directions depending on LTR, flyout layout, etc.)

@gonfunko
Copy link
Contributor Author

To be clear, you want separate LeftAction, RightAction, UpAction and DownAction classes/files?

Copy link
Collaborator

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, you want separate LeftAction, RightAction, UpAction and DownAction classes/files?

No, not at all. I just think we should rename the actions (id, name, description, etc.) to up/down/left/right, rather than prev/next/out/in.

(I think we should also rename the corresponding methods on the cursors, but that can be done as part of a subsequent PR that actually makes them RTL / horizontal flyout aware.)

@gonfunko
Copy link
Contributor Author

Got it, thanks for clarifying! Updated accordingly (and also removed other deleted actions from SHORTCUT_NAMES).

@gonfunko gonfunko merged commit 219eb37 into main Mar 13, 2025
1 check passed
@gonfunko gonfunko deleted the arrow-keys branch March 13, 2025 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move previous/next/in/out shortcuts to a new file

4 participants