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

As/search suggestion for engine selection #70

Merged
merged 9 commits into from
Jun 28, 2024

Conversation

soncuteanca
Copy link
Collaborator

Description

This verifies that when entering "@" in awesome bar, a list of search engines is suggested, and upon selecting an option, the search is performed using the selected engine.

Bugzilla bug ID

ID: 1902767
Link: https://bugzilla.mozilla.org/show_bug.cgi?id=1902767

Type of change

Please delete options that are not relevant.

  • [ x] New Test

How does this resolve / make progress on that bug?

Finished

Screenshots / Explanations

Please upload any relevant media or add a relevant description with respect to the bug listed above.

Comments / Concerns

Please add a short blurb about any comments or concerns that this change might cause.

@soncuteanca soncuteanca changed the title as/search suggestion for engine selection As/search suggestion for engine selection Jun 18, 2024
Copy link
Collaborator

@Tracy-Walker Tracy-Walker left a comment

Choose a reason for hiding this comment

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

Hi Anca, you use then re-use same code in lines 21 and 28. Suggest defining a function with that code and calling the function at each place it's needed.

Otherwise, this looks good!

Copy link
Collaborator

@ben-c-at-moz ben-c-at-moz left a comment

Choose a reason for hiding this comment

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

One thing to try, but it looks good!

modules/data/navigation.components.json Outdated Show resolved Hide resolved
@ben-c-at-moz
Copy link
Collaborator

@soncuteanca I'm still getting a failure on the test when trying for Amazon.com -- perhaps it's actually looking for just "Amazon"? Not sure...

Copy link
Collaborator

@ben-c-at-moz ben-c-at-moz left a comment

Choose a reason for hiding this comment

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

I'm sorry, it's still flaky for me...

@ben-c-at-moz
Copy link
Collaborator

One more thing, you're going to need to merge main before I'll be able to approve this one (not just resolve merge conflicts). Thanks!

@ben-c-at-moz ben-c-at-moz self-requested a review June 27, 2024 22:46
Copy link
Collaborator

@ben-c-at-moz ben-c-at-moz left a comment

Choose a reason for hiding this comment

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

I just added a little something and now it works without hard waits or iterative checks

@soncuteanca
Copy link
Collaborator Author

With the latest changes, I have achieved a 15/15 pass rate locally. Is there anything else I should do here?

@ben-c-at-moz
Copy link
Collaborator

@soncuteanca No, you just needed to resolve all the conversations by pressing the "Resolve Conversation". It's good to merge now

@soncuteanca soncuteanca merged commit d262b0e into main Jun 28, 2024
3 checks passed
@soncuteanca soncuteanca deleted the as/search-suggestion-for-engine-selection branch June 28, 2024 16:32
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