-
Notifications
You must be signed in to change notification settings - Fork 894
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
Remove ts-nocheck from brave://settings files #27107
Conversation
A Storybook has been deployed to preview UI for the latest push |
...me-browser-resources-settings-clear_browsing_data_dialog-clear_browsing_data_dialog.ts.patch
Outdated
Show resolved
Hide resolved
patches/chrome-browser-resources-settings-page_visibility.ts.patch
Outdated
Show resolved
Hide resolved
patches/chrome-browser-resources-settings-settings_menu-settings_menu.ts.patch
Outdated
Show resolved
Hide resolved
browser/resources/settings/brave_reset_page/brave_reset_profile_dialog_behavior.ts
Outdated
Show resolved
Hide resolved
...esources/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_on_exit_page.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, great work on this @emerick - that was a lot of thankless work.
LGTM % nits
...urces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.ts
Show resolved
Hide resolved
...urces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.ts
Show resolved
Hide resolved
...urces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.ts
Show resolved
Hide resolved
...urces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.ts
Show resolved
Hide resolved
...urces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.ts
Outdated
Show resolved
Hide resolved
browser/resources/settings/default_brave_shields_page/brave_adblock_browser_proxy.ts
Outdated
Show resolved
Hide resolved
browser/resources/settings/default_brave_shields_page/brave_adblock_browser_proxy.ts
Outdated
Show resolved
Hide resolved
browser/resources/settings/default_brave_shields_page/brave_adblock_browser_proxy.ts
Outdated
Show resolved
Hide resolved
patches/chrome-browser-resources-settings-page_visibility.ts.patch
Outdated
Show resolved
Hide resolved
ui/webui/resources/cr_components/certificate_manager/brave_overrides/certificate_manager_v2.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
5e704f0
to
fe8f786
Compare
fe8f786
to
4690527
Compare
[puLL-Merge] - brave/brave-core@27107 DescriptionThis PR makes significant changes to the Brave browser's settings pages, primarily focusing on TypeScript improvements and code modernization. It updates various components, adds type annotations, and refactors code to improve type safety and readability. Possible Issues
Security Hotspots
ChangesChanges
sequenceDiagram
participant User
participant Settings
participant BraveProxy
participant ChromeAPI
User->>Settings: Open Settings
Settings->>BraveProxy: Initialize proxies
BraveProxy->>ChromeAPI: Get initial settings
ChromeAPI-->>BraveProxy: Return settings
BraveProxy-->>Settings: Update UI
User->>Settings: Change setting
Settings->>BraveProxy: Update setting
BraveProxy->>ChromeAPI: Apply setting change
ChromeAPI-->>BraveProxy: Confirm change
BraveProxy-->>Settings: Update UI
|
4690527
to
c45e471
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patches
++
Released in v1.75.153 |
Resolves brave/brave-browser#31715
This PR removes
@ts-nocheck
from all brave://settings files, allowing type-checking to be enforced consistently there.I tried to add and use appropriate types everywhere it was feasible and pragmatic, but in a few situations I was forced to use
any
in order to avoid making unreasonably large changes. I also didn't try to convert the one or two legacy behaviors we still use into mixins.One of our existing settings-related patches need to be updated to include new routes/identifiers. Other than that, no patches were introduced.
It's best to review these by commit. The cosmetic/linter commit can largely be ignored, I just tried to bring our code somewhat up to the standards currently defined by our linter (I managed to eliminate about 50% of the linter-related errors with that commit).
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
This requires full testing of brave://settings, but changes primarily occurred in the following areas: