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

[Gtk4Prep] SearchResults: Use EventControllers, disambiguate view name #2402

Merged
merged 10 commits into from
Mar 5, 2024

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Jan 31, 2024

No description provided.

@jeremypw jeremypw marked this pull request as ready for review February 2, 2024 16:14
@jeremypw jeremypw requested a review from a team February 11, 2024 13:16
@zeebok
Copy link
Contributor

zeebok commented Feb 22, 2024

Code looks good, I will give this a test tomorrow!

@zeebok
Copy link
Contributor

zeebok commented Feb 22, 2024

Not sure if these fall under this exactly but I did notice some behaviors (at least when testing on OS8) that I am not sure are intentional with the search results

  • When there is only one result, if you press the down arrow key like you would to change the highlighted result it simply closes the search results.
  • If there are many results, pressing the down arrow at the last entry does not close like with single results, it cycles back to the first
  • If there are many results, pressing the up arrow at the first entry does cycle to the bottom or do anything

Copy link
Contributor

@zeebok zeebok left a comment

Choose a reason for hiding this comment

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

Code looks good and is what I see is reflected in testing. Not sure we want to address the behaviors I brought up here or save it for another PR. Up to you!

@jeremypw jeremypw merged commit 7b78fa0 into main Mar 5, 2024
4 checks passed
@jeremypw jeremypw deleted the jeremypw/gtk4prep-searchresults-events branch March 5, 2024 13:05
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.

3 participants