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

AI Chat Entry point B - Address bar #3694

Merged
merged 12 commits into from
Dec 9, 2024
Merged

AI Chat Entry point B - Address bar #3694

merged 12 commits into from
Dec 9, 2024

Conversation

Bunn
Copy link
Contributor

@Bunn Bunn commented Dec 6, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1204167627774280/1208794359260769/f

Description:
Add AI Chat to the Omnibar accessory button

Steps to test this PR:

  1. Make sure you're set as internal user
  2. Go to a SERP, check that instead of the share button you'll see the chat button
  3. Check if the chat button works
  4. Open any site from the SERP, check if the button changes to the share button
  5. Check if the share button works
  6. Go to settings, disable the AI Chat in search results
  7. Do the same steps above, make sure we never display the chat button
  1. Do a clean install, do not make yourself an internal user.
  2. Validate that you do not see the chat icon there

@Bunn Bunn requested a review from brindy December 6, 2024 21:17
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2024 •
edited
Loading

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 8c25e8d

brindy
brindy requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@brindy brindy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good, but a few minor things...

  • rename onShareLongPress -> onAccessoryLongPress for consistency
  • SwiftLint (was flagging the wrong line 🙄 )
  • typo of the showAccessoryButton var in the omni bar state

DuckDuckGo/OmniBarDelegate.swift Outdated Show resolved Hide resolved
@Bunn Bunn requested a review from brindy December 9, 2024 15:47
@Bunn
Copy link
Contributor Author

Bunn commented Dec 9, 2024 •
edited
Loading

@brindy I've addressed all the suggestions, thanks for it!
As we discussed on MM, here are some additional changes:

  • Added a conditional UI for settings using the feature flags and also fixed some settings issues -> 96e1e5a
  • New tests related to the issues fixed above -> 07c80c8
  • Small rename to make the property names more consistent -> c492804
  • Fix that issue related to not dismissing when opening a not-AI Chat link -> 8c25e8d

If you want to test different privacy configs:

brindy
brindy approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM!

@Bunn Bunn merged commit 7d77c7d into main Dec 9, 2024
13 checks passed
@Bunn Bunn deleted the bunn/aichat/share-entry-point branch December 9, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brindy brindy brindy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Bunn @brindy