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

[Material]: Enable tri color themes from upstream, now Nala supports them #25186

Closed
wants to merge 8 commits into from

Conversation

fallaciousreasoning
Copy link
Contributor

Colors should trickle through from the Material UI theme in the builds from this PR. CC @aguscruiz could you take a look at the build from this PR?

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Copy link
Contributor

[puLL-Merge] - brave/brave-core@25186

Description

This PR modifies the color system in Brave's UI, primarily focusing on simplifying and standardizing color usage across different themes (light, dark, and private). It removes some custom color definitions and replaces them with standardized color values from a nala color system.

Changes

Changes

  1. browser/ui/color/brave_color_mixer.cc:

    • Removed several custom color constants and replaced them with nala color system values.
    • Simplified color mixing logic for omnibox and sidebar elements.
    • Removed separate light and dark theme color mixers, consolidating them into a single mixer.
    • Updated color definitions for various UI elements to use nala color system.
  2. chromium_src/chrome/browser/ui/webui/cr_components/theme_color_picker/theme_color_picker_handler.cc:

    • Removed this file, likely because the custom theme color picker logic is no longer needed.
  3. chromium_src/ui/color/ui_color_mixer.cc:

    • Updated button foreground color to use nala color system.
  4. Removed patches:

    • Removed several patch files related to theme color picker and UI color definitions, suggesting that these customizations are no longer necessary due to the new color system.
  5. test/filters/unit_tests.filter:

    • Removed disabled tests related to ThemeColorPickerHandler, as these are likely no longer relevant with the new color system.

Possible Issues

  1. The removal of custom color definitions and consolidation of color mixers might lead to unexpected visual changes in some parts of the UI.
  2. The elimination of separate light and dark theme color mixers could potentially reduce fine-grained control over theme-specific colors.

Security Hotspots

No significant security issues are apparent in this change. The modifications primarily affect visual styling and do not seem to introduce any new attack surfaces or security vulnerabilities.

@fallaciousreasoning
Copy link
Contributor Author

Closing, as fixed by #25337

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant