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

Move Look & feel section to Shields prefs page #5964

Merged
merged 1 commit into from
Jul 8, 2020
Merged

Move Look & feel section to Shields prefs page #5964

merged 1 commit into from
Jul 8, 2020

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jun 29, 2020 •
edited
Loading

Look & feel section is moved to the top to include choosing Shields default view.

Resolves brave/brave-browser#10502

Screen Shot 2020-06-29 at 9 53 07 AM

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.

@simonhong simonhong added this to the 1.12.x - Nightly milestone Jun 29, 2020
@simonhong simonhong self-assigned this Jun 29, 2020
@simonhong simonhong force-pushed the modify_look_and_feel_section_in_shields_prefs branch from 7649e80 to 30203b3 Compare June 29, 2020 01:10
@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jun 29, 2020
@simonhong simonhong requested a review from bsclifton June 29, 2020 05:15
@petemill
Copy link
Member

If this can wait until after cr84 has landed in master, that would be great.

@simonhong simonhong removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jun 30, 2020
@simonhong
Copy link
Member Author

@petemill can wait :)

@simonhong simonhong force-pushed the modify_look_and_feel_section_in_shields_prefs branch from 30203b3 to aa44d75 Compare July 3, 2020 14:51
@simonhong
Copy link
Member Author

@petemill Rebased on cr84 :)

petemill
petemill approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Minor naming suggestion and nit, after that's changed all looks good 👍

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html Outdated
class="continuation"
pref="{{prefs.brave.shields.stats_badge_visible}}"
label="$i18n{showStatsBlockedBadgeLabel}">

Copy link
Member

Choose a reason for hiding this comment

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

nit: empty line

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html Outdated
margin: 0 20px;
cursor: pointer;
}
.default-view-padding {
Copy link
Member

Choose a reason for hiding this comment

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

maybe give it a more general name, not descriptive of the rules inside. Like default-view-item? That's in case someone adds more rules then the class name would not match what it does. It would also perhaps be more readable to put the class block before its children (shields-view-col).

Copy link
Member Author

Choose a reason for hiding this comment

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

All done.

@petemill petemill force-pushed the modify_look_and_feel_section_in_shields_prefs branch from aa44d75 to 3e7d784 Compare July 7, 2020 22:11
@simonhong
Move Look & feel section to Shields prefs page
58d2112 
Look & feel section is moved to the top to include choosing Shields default view.
@simonhong simonhong force-pushed the modify_look_and_feel_section_in_shields_prefs branch from 3e7d784 to 58d2112 Compare July 7, 2020 23:16
@simonhong
Copy link
Member Author

@petemill Thanks for review!

@simonhong
Copy link
Member Author

Merged because only test-install step is failed.

@simonhong simonhong merged commit 13ba233 into master Jul 8, 2020
@simonhong simonhong deleted the modify_look_and_feel_section_in_shields_prefs branch July 8, 2020 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petemill petemill petemill approved these changes

@karenkliu karenkliu Awaiting requested review from karenkliu

@rebron rebron Awaiting requested review from rebron

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.12.x - Release #1
Development

Successfully merging this pull request may close these issues.

[Desktop] Modify Look & feel section in brave://settings/shields
2 participants
@simonhong @petemill