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.

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

feat: add keyboard navigation to attributions and signals lists #2583

Merged
merged 3 commits into from Mar 6, 2024
Merged

feat: add keyboard navigation to attributions and signals lists #2583

merged 3 commits into from Mar 6, 2024

Conversation

mstykow
Copy link
Collaborator

@mstykow mstykow commented Feb 23, 2024

Summary of changes

  • merge PackageCard and ListCard components
  • track keyboard selected item in list and grouped-list
  • add focus styling to package card

Context and reason for change

closes #971

How can the changes be tested

Check keyboard navigation after clicking on a package card in the attributions or signals lists. Pay special attention also to switching tabs: the previously focused package card should not carry over to another tab.

@mstykow mstykow linked an issue Feb 23, 2024 that may be closed by this pull request
Enable keyboard navigation in package lists #971
Closed
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 037c64d to 84f2f8b Compare February 24, 2024 09:19
MarkusObendrauf
MarkusObendrauf approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@MarkusObendrauf MarkusObendrauf left a comment

Choose a reason for hiding this comment

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

Cool. Sometimes using arrow keys is a bit slow (still less that 0.5s but its noticeable). For example, if you're on the second-last item and press down, it takes a while for the highlighted item to move.

src/Frontend/util/use-virtuoso-refs.ts Outdated Show resolved Hide resolved
src/e2e-tests/__tests__/selecting-attributions.test.ts Show resolved Hide resolved
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 2 times, most recently from 4abb826 to bcf883d Compare February 26, 2024 11:54
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 84f2f8b to a6c9ea0 Compare February 26, 2024 12:38
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from a6c9ea0 to 065f2d9 Compare February 26, 2024 13:06
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 5 times, most recently from 049dc28 to 4be36e9 Compare February 26, 2024 21:19
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 065f2d9 to 7ca8c19 Compare February 26, 2024 21:23
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 3 times, most recently from ae99e47 to fbf99e6 Compare February 27, 2024 12:31
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 7ca8c19 to 45258c4 Compare February 27, 2024 12:32
MarkusObendrauf
MarkusObendrauf requested changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@MarkusObendrauf MarkusObendrauf left a comment

Choose a reason for hiding this comment

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

Hey, finally figured out that a bug I'm encountering in the follow-up already exists in this version.

To reproduce:

  1. Select a resource without attributions
  2. Focus on the attributions panel, eg. by clicking on the scrollbar
  3. Scroll down using the down arrow
  4. Select a different resource without attributions
  5. Focus on the attributions panel, eg. by clicking on the scrollbar
  6. Scroll down using the down arrow

Notice that the focused index was preserved from the previous list.

@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 3 times, most recently from e49bc07 to 2ec903d Compare March 4, 2024 10:33
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 45258c4 to 9edb3b3 Compare March 4, 2024 12:11
@mstykow
Copy link
Collaborator Author

mstykow commented Mar 4, 2024

Hey, finally figured out that a bug I'm encountering in the follow-up already exists in this version.

To reproduce:

  1. Select a resource without attributions
  2. Focus on the attributions panel, eg. by clicking on the scrollbar
  3. Scroll down using the down arrow
  4. Select a different resource without attributions
  5. Focus on the attributions panel, eg. by clicking on the scrollbar
  6. Scroll down using the down arrow

Notice that the focused index was preserved from the previous list.

I think I found a pretty simple solution to this issue that should also help you with the focus-stealing problems you faced in your follow-up task.

@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 9edb3b3 to 9563b34 Compare March 4, 2024 12:14
MarkusObendrauf
MarkusObendrauf requested changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@MarkusObendrauf MarkusObendrauf left a comment •
edited

Choose a reason for hiding this comment

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

It's still a little buggy:

  1. Click on a resource with no attached attributions
  2. Click on an attribution
  3. Click on a resource with an attached attribution
  4. Click on a resource with no attached attributions
    Notice that the focus index from step 2 was preserved.

MarkusObendrauf
MarkusObendrauf approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@MarkusObendrauf MarkusObendrauf left a comment

Choose a reason for hiding this comment

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

Seems to work, I'm just praying that I never have to open up use-virtuoso-refs.ts again.

@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch 2 times, most recently from 9c8afb8 to ebd09c6 Compare March 4, 2024 14:59
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 2 times, most recently from c9b54aa to a0de24d Compare March 4, 2024 18:56
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from ebd09c6 to 3f7f9f4 Compare March 4, 2024 18:59
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 3 times, most recently from 72eb751 to 7c556e9 Compare March 5, 2024 13:38
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch 3 times, most recently from 2b34713 to 109cd84 Compare March 5, 2024 16:28
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 4 times, most recently from 99140be to f2c7fc3 Compare March 5, 2024 19:33
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 109cd84 to b95ffda Compare March 5, 2024 19:35
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from b95ffda to 18d42f1 Compare March 6, 2024 10:08
@mstykow mstykow force-pushed the feat-opossumui-2.0 branch 2 times, most recently from 3c02b80 to d26f86d Compare March 6, 2024 16:42
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 18d42f1 to 29cd131 Compare March 6, 2024 16:45
@mstykow
feat: add keyboard navigation to attributions and signals lists
50a0462 
- merge PackageCard and ListCard components
- track keyboard selected item in list and grouped-list
- add focus styling to package card

closes #971

Signed-off-by: Maxim Stykow <maxim.stykow@tngtech.com>
@mstykow mstykow force-pushed the feat-list-keyboard-navigation branch from 29cd131 to 50a0462 Compare March 6, 2024 17:05
@mstykow
feat: add keyboard navigation for resource tree
91bf5fa 
- keep track of focused ID instead of focused index in Virtuoso
- add focus state for tree nodes and define keyboard actions

closes #2572

Signed-off-by: Maxim Stykow <maxim.stykow@tngtech.com>
Signed-off-by: Markus Obendrauf <markus.obendrauf@tngtech.com>
Base automatically changed from feat-opossumui-2.0 to main March 6, 2024 17:19
@mstykow
Merge pull request #2591 from opossum-tool/feat-tree-keyboard-navigation
23173fd 
feat: add keyboard navigation for resource tree
@mstykow mstykow merged commit 3748d74 into main Mar 6, 2024
5 checks passed
@mstykow mstykow deleted the feat-list-keyboard-navigation branch March 6, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkusObendrauf MarkusObendrauf MarkusObendrauf approved these changes

Assignees

@MarkusObendrauf MarkusObendrauf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable keyboard navigation in package lists
2 participants
@mstykow @MarkusObendrauf