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

[reopen] [cr84] Settings page follow-ups: fixes guest mode, and refactors to overrides by module name #5985

Merged
merged 4 commits into from
Jul 2, 2020

Conversation

petemill
Copy link
Member

@petemill petemill commented Jul 1, 2020

Reopening as #5976 was reverted in cr84. Android build was failing due to trying to minify the JS which was included as binary data (or maybe it was compressed). But now we have merged to master a change that prevents android compiling settings webui, which was never intended for android.

  • Fix settings page in guest mode windows, use and override page visibility correctly
  • Brings back the row divider lines for brave items.
  • Refactor all settings webui overrides to be in modules with the name of the overriden component, as per the new README.md:
Use this directory to add modifications to any chromium polymer module.
Steps to take:
- Create a JS module which calls any of the polymer_overriding exported functions, e.g. `RegisterStyleOverride`.
- Import the module to the index.js inside this directory.
- Include the module in settings_resources.grd.

Fix brave/brave-browser#5065
Fix brave/brave-browser#2995

image

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@petemill petemill requested review from mkarolin and bridiver July 1, 2020 04:59
@petemill petemill self-assigned this Jul 1, 2020
@mkarolin mkarolin force-pushed the cr84 branch 2 times, most recently from 268ce7b to 4ee0d6e Compare July 1, 2020 22:12
petemill added 4 commits July 1, 2020 15:18
Now that everything is defined in JS (with the move to es-modules), we can more declaratively define the overrides. This gives us an opportunity to make the overrides more readable, maintainable and expressed in the expected file name or the overridden component.
Use and override page visibility correctly
@petemill petemill force-pushed the cr84-settings-refactor branch from bd285ad to 3435743 Compare July 1, 2020 22:18
Base automatically changed from cr84 to master July 2, 2020 01:07
@petemill petemill requested a review from bsclifton July 2, 2020 16:38
@petemill
Copy link
Member Author

petemill commented Jul 2, 2020

Everything CI passed, except audit_deps which is marked as unstable

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

++

@petemill petemill merged commit 395bae2 into master Jul 2, 2020
@petemill petemill deleted the cr84-settings-refactor branch July 2, 2020 22:11
mkarolin pushed a commit that referenced this pull request Jul 7, 2020
[reopen] [cr84] Settings page follow-ups: fixes guest mode, and refactors to overrides by module name
@bsclifton
Copy link
Member

Uplifted to 1.11 with #5996

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