Skip to content

[PM-21724] - add safari and firefox to list of potential browser vendors #14857

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

Merged
merged 3 commits into from
May 29, 2025
+15 −2
Merged

[PM-21724] - add safari and firefox to list of potential browser vendors #14857

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11e5a39
add safari and firefox to list of potential browser vendors
jaasen-livefront May 20, 2025
ad68534
use browserClientVendorExtended
jaasen-livefront May 20, 2025
4dd669f
handle unknown browser client vendor
jaasen-livefront May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions apps/browser/src/autofill/popup/settings/autofill.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,20 @@
return null;
}

get spotlightButtonText() {
if (this.browserClientVendor === BrowserClientVendors.Unknown) {
return this.i18nService.t("turnOffAutofill");
get browserClientVendorExtended() {
if (this.browserClientVendor !== BrowserClientVendors.Unknown) {
return this.browserClientVendor;
Copy link
Contributor

@jprusik jprusik May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i18n nit:

This has potential to not be a properly formatted string for interpolation if the BrowserClientVendors constant values are changed later (it's not clear to me that the strings in BrowserClientVendors are intended to be directly user-facing), which is why I think returning the strings for the Firefox/Safari cases makes sense; we'd probably ideally do the same for each of the other cases in this string-builder for this component.

Sidenote: expressing in the getter name that the values returned are intended to be user-facing strings could be helpful (e.g. browserClientVendorExtended -> browserVendorNameText).

That said, I think we can forgo that level of defensiveness for now and come back to it if/when it becomes a concern.

}

Check warning on line 339 in apps/browser/src/autofill/popup/settings/autofill.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/popup/settings/autofill.component.ts#L339 

Added line #L339 was not covered by tests
if (this.platformUtilsService.isFirefox()) {
return "Firefox";
}

Check warning on line 342 in apps/browser/src/autofill/popup/settings/autofill.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/popup/settings/autofill.component.ts#L342 

Added line #L342 was not covered by tests
if (this.platformUtilsService.isSafari()) {
return "Safari";
}
return this.i18nService.t("turnOffBrowserAutofill", this.browserClientVendor);
}

get spotlightButtonText() {
return this.i18nService.t("turnOffBrowserAutofill", this.browserClientVendorExtended);
}

async dismissSpotlight() {
Expand Down
Loading