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

Update bookmarks toolbar folder icons #10037

Merged
merged 1 commit into from
Sep 11, 2021
Merged

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Sep 10, 2021

  • Windows icons are back to yellow
  • macOS and Linux use a gray colored icon (Linux has slightly different dimensions)

Having a proper icon solves the visibility issue for sub-folders on private window.

Fixes brave/brave-browser#17463
Fixes brave/brave-browser#17214

Test plan (needs to be ran on macOS, Windows, and Linux)

  1. New profile
  2. Hamburger menu => Bookmarks => Show bookmarks
  3. Add a few folders to the bookmarks toolbar
  4. Add a few subfolders (to cover test plan in Follow up of #16940 - Brave branded bookmarks subfolder images are not shown clearly in Private tabs brave-browser#17214)
  5. Verify images look like below (see each OS screenshot) in Regular window
  6. Opening a new Private window should look like dark mode
  7. Sub-folders in bookmarks toolbar in private mode should be like light mode

Windows

image
image

macOS

Screen Shot 2021-09-11 at 9 05 41 AM

Screen Shot 2021-09-11 at 9 05 58 AM

Linux

Screenshot from 2021-09-11 09-26-45
Screenshot from 2021-09-11 09-27-14

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • 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, npm run lint, 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:

@bsclifton bsclifton requested a review from a team as a code owner September 10, 2021 08:08
@bsclifton bsclifton self-assigned this Sep 10, 2021
@bsclifton bsclifton changed the title Update folder icons WIP: Update folder icons Sep 10, 2021
- Windows icons are back to yellow
- macOS and Linux use a gray colored icon (Linux has slightly different
  dimensions)

Having a proper icon solves the visibility issue for sub-folders on
private window.

Fixes brave/brave-browser#17463
Fixes brave/brave-browser#17214
@bsclifton bsclifton changed the title WIP: Update folder icons Update folder icons Sep 10, 2021
@bsclifton bsclifton changed the title Update folder icons Update bookmarks toolbar folder icons Sep 10, 2021
@bsclifton
Copy link
Member Author

Ready for review! 😄 @simonhong @mariospr would you all be able to help me get a screenshot for Linux / macOS? (light mode, dark mode). You can edit the top post and share there

Copy link

@karenkliu karenkliu left a comment

Choose a reason for hiding this comment

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

Windows looks as expected! 🚀

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.

LGTM

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++ - attached screenshots from macOS and linux

@kjozwiak
Copy link
Member

kjozwiak commented Sep 13, 2021

Test Case 1 - brave/brave-browser#17463

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.31.35 Chromium: 93.0.4577.63 (Official Build) nightly (64-bit)
-- | --
Revision | ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS | Windows 10 OS Version 2009 (Build 19042.1165)

Ensured that the bookmark icons are matching the ones via #10037 (comment) when upgrading and installing a new profile. Examples:

Upgrade Case

Example Example
beforeUpdate afterUpdate

Clean Install

Example Example
lightTheme darkTheme
lightTheme2 darkTheme2

Verification PASSED on PopOS 21.04 x64 using the following build:

Brave | 1.31.35 Chromium: 93.0.4577.63 (Official Build) nightly (64-bit)
--- | ---
Revision | ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS | Linux

Ensured that the bookmark icons are matching the ones via #10037 (comment) when upgrading and installing a new profile. Examples:

Upgrade Case

Example Example
beforeUpdate afterUpgrade

Clean Install

Example Example
lightTheme darkTheme

Verification PASSED on macOS 11.5.2 x64 using the following build:

Brave | 1.31.35 Chromium: 93.0.4577.63 (Official Build) nightly (x86_64)
--- | ---
Revision | ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS | macOS Version 11.5.2 (Build 20G95)

Ensured that the bookmark icons are matching the ones via #10037 (comment) when upgrading and installing a new profile. Examples:

Upgrade Case

Example Example
Screen Shot 2021-09-12 at 10 58 36 PM Screen Shot 2021-09-12 at 11 00 11 PM

Clean Install

Example Example
Screen Shot 2021-09-12 at 11 01 13 PM Screen Shot 2021-09-12 at 11 01 37 PM

Test Case 2 - brave/brave-browser#17214

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.31.35 Chromium: 93.0.4577.63 (Official Build) nightly (64-bit)
-- | --
Revision | ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS | Windows 10 OS Version 2009 (Build 19042.1165)
Private Window Tor Window
privateTab torTab

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