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

[Settings] Fixes fonts subpage flakiness in search. #23064

Merged
merged 1 commit into from
Apr 15, 2024
Merged

[Settings] Fixes fonts subpage flakiness in search. #23064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions browser/resources/settings/brave_content_page/content.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,12 @@
label="$i18n{tabsToLinks}">
</settings-toggle-button>
</if>
<settings-animated-pages id="pages" section="content">
<template is="dom-if" route-path="/fonts">
<settings-subpage associated-control="[[$$('#customize-fonts-subpage-trigger')]]"
page-title="$i18n{customizeFonts}">
<settings-appearance-fonts-page prefs="{{prefs}}">
</settings-appearance-fonts-page>
</settings-subpage>
</template>
</settings-animated-pages>
7 changes: 7 additions & 0 deletions browser/resources/settings/brave_overrides/appearance_page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ RegisterPolymerTemplateModifications({
} else {
customizeFontsSubpageTrigger.remove()
}
const customizeFontsTemplate = templateContent.querySelector(
'template[is=dom-if][route-path="/fonts"]')
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Suggested change
'template[is=dom-if][route-path="/fonts"]')
'template[is=dom-if][route-path="/braveContents/fonts"]')

if (!customizeFontsTemplate) {
console.error(`[Brave Settings Overrides] Couldn't find customize fonts subpage template`)
} else {
customizeFontsTemplate.remove()
}
const pageZoom = templateContent.querySelector('.cr-row:has(#pageZoom)')
if (!pageZoom) {
console.error(`[Brave Settings Overrides] Couldn't find page zoom`)
Expand Down
8 changes: 8 additions & 0 deletions browser/resources/settings/brave_overrides/people_page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ RegisterPolymerTemplateModifications({
} else {
console.error('[Brave Settings Overrides] People Page cannot find syncSetup/advanced template')
}
const syncSetupPageContent = templateContent.querySelector(
Copy link
Member

Choose a reason for hiding this comment

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

Q) It's also leftover and could be deleted as we have our own sync page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we don't seem to be using this page. It seems to have something to do with the Privacy Guide, which we disable anyway. When using search in settings, if not removed, this template causes an exception because it can't find the trigger control for the page.

'template[is=dom-if][route-path="/syncSetup/pageContent"]')
if (syncSetupPageContent) {
syncSetupPageContent.remove()
} else {
console.error('[Brave Settings Overrides] People Page cannot ' +
'find syncSetup/pageContent template')
}
// always show the template content
signinTemplate.setAttribute('if', 'true')
// remove the google account button
Expand Down
5 changes: 5 additions & 0 deletions browser/resources/settings/brave_routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ export default function addBraveRoutes(r: Partial<SettingsRoutes>) {
}
if (pageVisibility.content) {
r.BRAVE_CONTENT = r.BASIC.createSection('/braveContent', 'content')
// Move fonts from APPEARANCE to BRAVE_CONTENT
if (r.FONTS) {
delete r.FONTS
}
r.FONTS = r.BRAVE_CONTENT.createChild('/fonts');
Copy link
Member

Choose a reason for hiding this comment

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

How about having brave://settings/braveContent/fonts instead of brave://settings/fonts?

Suggested change
r.FONTS = r.BRAVE_CONTENT.createChild('/fonts');
r.FONTS = r.BRAVE_CONTENT.createChild('/braveContent/fonts');

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I personally have no objections. The only thing I wonder if people who are used to going to brave://settings/fonts would be unhappy with the URL change.

Copy link
Member

Choose a reason for hiding this comment

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

ah, i see. Using current one would be helpful for many users.

}
if (r.SITE_SETTINGS) {
r.SITE_SETTINGS_AUTOPLAY = r.SITE_SETTINGS.createChild('autoplay')
Expand Down
Loading