Skip to content

DYN-8623 : improve accessibility #16237

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.

Sign up for GitHub

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

Jump to bottom
Merged
BogdanZavu merged 1 commit into from
May 28, 2025
Merged

DYN-8623 : improve accessibility #16237

BogdanZavu merged 1 commit into from
May 28, 2025

Conversation

@BogdanZavu
Copy link
Contributor

@BogdanZavu BogdanZavu commented May 21, 2025 •
edited by johnpierson
Loading

Purpose

Handle enter and arrow keys in the new autocomplete. For the old one the improvement is included in this pr.

Declarations

Check these if you believe they are true

  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB
  • This PR introduces new feature code involve network connecting and is tested with no-network mode.

Release Notes

Addition of keyboard navigation in the Node AutoComplete bar.

  • Pressing Up/Left or Down/Right arrow keys now moves the selection within the auto-complete suggestions.
  • Pressing Enter or Escape now properly closes the auto-complete and prevents further event propagation.

Reviewers

@DynamoDS/synapse

github-actions[bot]
github-actions bot reviewed May 21, 2025
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8623

@johnpierson johnpierson requested a review from Copilot May 27, 2025 13:36
Copilot AI reviewed May 27, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds keyboard navigation support (Enter and arrow keys) to the new autocomplete component and removes an unused import.

  • Handle Escape, Enter, Up/Left, and Down/Right keys by moving selection and marking events as handled.
  • Remove unused using Dynamo.ViewModels; directive.
Files not reviewed (1)
  • src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml: Language not supported
Comments suppressed due to low confidence (1)

src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml.cs:118

  • New key handling logic for Enter and arrow keys isn’t covered by existing tests. Please add unit or integration tests to verify navigation index changes and that events are marked as handled.
e.Handled = true;

src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml.cs
{
case Key.Escape:
CloseAutoComplete();
e.Handled = true;
Copy link

Copilot AI May 27, 2025

Choose a reason for hiding this comment

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

There’s repeated e.Handled = true; in each case branch. Consider using a single flag to mark handled once or consolidate common logic after the switch to reduce duplication.

Copilot uses AI. Check for mistakes.
johnpierson

This comment was marked as outdated.

Sign in to view

@BogdanZavu
Copy link
Contributor Author

BogdanZavu commented May 27, 2025 •
edited
Loading

It seems like the enter key doesn't place the cluster that I am hovering on with the arrow keys.

Maybe an issue with the build ? When using arrow keys it changes the preview cluster.

@johnpierson
Copy link
Member

johnpierson commented May 27, 2025

It seems like the enter key doesn't place the cluster that I am hovering on with the arrow keys.

Maybe an issue with the build ? When using arrow keys it changes the preview cluster.

Let me rebuild and retest

johnpierson
johnpierson approved these changes May 27, 2025
View reviewed changes
Copy link
Member

@johnpierson johnpierson left a comment

Choose a reason for hiding this comment

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

holy smokes. this works awesome. other than that merge conflict. this lgtm. Will show this in demo today I think.

@BogdanZavu BogdanZavu merged commit b0e9f83 into DynamoDS:master May 28, 2025
25 of 27 checks passed
@johnpierson johnpierson added the ai/ml 🧠 synapse team related PRs label Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

Copilot code review Copilot Copilot left review comments

@johnpierson johnpierson johnpierson approved these changes

Assignees

No one assigned

Labels

ai/ml 🧠 synapse team related PRs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BogdanZavu @johnpierson @zavub