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

Fix incognito icon in cookie settings #7074

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

mherrmann
Copy link
Collaborator

@mherrmann mherrmann commented Nov 6, 2020

Fixes: brave/brave-browser#11738

@mihaiplesa can you suggest additional reviewers?

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.

@mherrmann mherrmann requested a review from mihaiplesa November 6, 2020 09:28
@mherrmann mherrmann requested a review from a team as a code owner November 6, 2020 09:28
@mherrmann
Copy link
Collaborator Author

mherrmann commented Nov 6, 2020

The Linux build failure seems to be unrelated to this PR. I tried the generated .deb and am getting the expected UI change:

image

image

@mihaiplesa
Copy link
Collaborator

That icon looks familiar, wonder if we already had it added and we can just reference it.

Screen Shot 2020-11-06 at 11 14 18 AM

@mherrmann
Copy link
Collaborator Author

I did a preliminary search for the existing icon while researching the fix. Will try again.

@mherrmann
Copy link
Collaborator Author

mherrmann commented Nov 6, 2020

It's available as incognito.icon and incognito_profile.icon in vector_icons/chrome/app/vector_icons.

In words, the "Private" icons in this screenshot in Brave:

image

are produced by the .icon files above. Here are screenshots of these .icon files, generated with https://github.com/sadrulhc/vector-icons:

image

image

I am not sure whether we can reference .icon files in brave://settings. Do you know who we could ask @mihaiplesa?

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.

Thanks for this, left some feedback that would result in not needing to patch.

@mherrmann
Copy link
Collaborator Author

Thank you very much for your kind review and suggestions @petemill @mkarolin. I implemented your recommended changes and still get the same screenshots:

image

image

I also verified that it uses the shorter SVG suggested by @petemill.

image

This is my first PR to brave-core. How should I proceed? For example, should I squash my commits, or rebase them onto the current master?

@mherrmann mherrmann requested a review from petemill November 9, 2020 11:01
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.

Looks perfect. Yes, please squash before merge if you are able.

You don't have to rebase on master as there are no conflicts and CI has passed enough steps apart from what seems like a transient error that IMO we can ignore.

Good to merge if your squash results in no code change.

@mherrmann mherrmann force-pushed the mherrmann-fix-incognito-icon-in-settings branch from 837699e to 10da270 Compare November 10, 2020 05:54
@mherrmann mherrmann merged commit df28dba into master Nov 10, 2020
@mherrmann mherrmann deleted the mherrmann-fix-incognito-icon-in-settings branch November 10, 2020 05:57
@mherrmann
Copy link
Collaborator Author

Squashed and merged. Thank you @petemill @mkarolin @mihaiplesa !

@bsclifton bsclifton added this to the 1.18.x - Nightly milestone Nov 10, 2020
@bsclifton
Copy link
Member

🕶️

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

Successfully merging this pull request may close these issues.

Chrome incognito icon is used in allow cookies sites list
5 participants