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

Fix keydown event for Search engines dropdown #483

Merged
merged 9 commits into from
Jan 19, 2025

Conversation

prem-k-r
Copy link
Collaborator

@prem-k-r prem-k-r commented Jan 18, 2025

📝 Description

Keyboard:

  • Clear selected state and reset index when dropdown opens
  • When enter is pressed switch to the selected search engine, close the dropdown and focus on the searchInput
  • Prevent scrolling when using the arrow keys

Mouse:

  • When switched to the selected search engine by clicking, close the dropdown and focus on the searchInput

Normal Search With | Mouse:

  • Focus on the searchInput for search engine radio button div click too

📸 Screenshots / 📹 Videos

None

🔗 Related Issues

None

✅ Checklist

  • I have read and followed the Contributing Guidelines.
  • I have tested my changes by installing them as an extension (not just running via localhost) to ensure it builds, installs, and works as expected.
  • I have tested these changes in at least Chrome and Firefox (other browsers if applicable).
  • Attached visual evidence of changes (screenshots or videos) if applicable.

Prevent the page from scrolling
mouse click close and focus
@prem-k-r prem-k-r marked this pull request as ready for review January 18, 2025 08:50
@prem-k-r prem-k-r self-assigned this Jan 18, 2025
@prem-k-r prem-k-r added enhancement New feature or request bug fixes If there is any problem there this will fix it labels Jan 18, 2025
@prem-k-r prem-k-r requested a review from itz-rj-here January 18, 2025 08:51
itz-rj-here
itz-rj-here previously approved these changes Jan 18, 2025
Copy link
Collaborator

@itz-rj-here itz-rj-here left a comment

Choose a reason for hiding this comment

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

The code looks right but i still need to check when I'm on my computer. 😅

@prem-k-r

This comment was marked as outdated.

searchInput focus for radio button div click too
Copy link
Collaborator

@itz-rj-here itz-rj-here left a comment

Choose a reason for hiding this comment

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

Well, as you told that preventing scroll when using arrow key, idk if it works as you said. Cause I can scroll with my mouse pointer when using arrow key 😅. But others looks good to me.
I guess you could include a feature with this improvement. Like adding, when we use the arrow key and stop's at a selected search suggestion, the search suggestion text will be also added at the searchbar too like the chrome one.

@prem-k-r
Copy link
Collaborator Author

Well, as you told that preventing scroll when using arrow key, idk if it works as you said. Cause I can scroll with my mouse pointer when using arrow key 😅. But others looks good to me.

Scrolling, you can resize the browser window to check, I meant whole screen UI moves when arrow up and down when height doesn't fit, unrelated to mouse.

I guess you could include a feature with this improvement. Like adding, when we use the arrow key and stop's at a selected search suggestion, the search suggestion text will be also added at the searchbar too like the chrome one.

I don't get it, do you mean showing search engines name in searchbar?

@itz-rj-here
Copy link
Collaborator

Well, as you told that preventing scroll when using arrow key, idk if it works as you said. Cause I can scroll with my mouse pointer when using arrow key 😅. But others looks good to me.

Scrolling, you can resize the browser window to check, I meant whole screen UI moves when arrow up and down when height doesn't fit, unrelated to mouse.

I guess you could include a feature with this improvement. Like adding, when we use the arrow key and stop's at a selected search suggestion, the search suggestion text will be also added at the searchbar too like the chrome one.

I don't get it, do you mean showing search engines name in searchbar?

Yup.

@prem-k-r
Copy link
Collaborator Author

Well, as you told that preventing scroll when using arrow key, idk if it works as you said. Cause I can scroll with my mouse pointer when using arrow key 😅. But others looks good to me.

Scrolling, you can resize the browser window to check, I meant whole screen UI moves when arrow up and down when height doesn't fit, unrelated to mouse.

THIS:

Untitled.video.-.Made.with.Clipchamp.3.mp4

I guess you could include a feature with this improvement. Like adding, when we use the arrow key and stop's at a selected search suggestion, the search suggestion text will be also added at the searchbar too like the chrome one.

I don't get it, do you mean showing search engines name in searchbar?

Yup.

Showing Google/Brave/Duck etc in place of Type here...? idk, I'll pass for now

Any bug in the codes?

@itz-rj-here
Copy link
Collaborator

This:

2025-01-18.19-30-20.mp4

@prem-k-r
Copy link
Collaborator Author

oh, nice I can try it after #479

@prem-k-r prem-k-r requested a review from itz-rj-here January 18, 2025 14:20
itz-rj-here
itz-rj-here previously approved these changes Jan 18, 2025
@prem-k-r prem-k-r added the ready to merge The pull request is ready to merge label Jan 18, 2025
@prem-k-r
Copy link
Collaborator Author

Roughly counted previous major PR after v3, updated in manifest version

@prem-k-r prem-k-r requested a review from itz-rj-here January 18, 2025 15:18
itz-rj-here
itz-rj-here previously approved these changes Jan 18, 2025
@prem-k-r
Copy link
Collaborator Author

@XengShi please check, if everything ok or I need to revert anything

@XengShi XengShi merged commit 3ca4621 into XengShi:main Jan 19, 2025
@prem-k-r prem-k-r deleted the keydown-search-engines branch January 19, 2025 17:56
@prem-k-r
Copy link
Collaborator Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fixes If there is any problem there this will fix it enhancement New feature or request ready to merge The pull request is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants