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

Empty icons for cards flip operations #8170

Merged
merged 1 commit into from
Mar 19, 2021
Merged

Empty icons for cards flip operations #8170

merged 1 commit into from
Mar 19, 2021

Conversation

AlexeyBarabash
Copy link
Contributor

Replaced the icons

drawable-hdpi/logo_card_back.png
drawable-mdpi/logo_card_back.png
drawable-xhdpi/logo_card_back.png

with empty stubs without Chromium icons.

If we want to put the Brave icons, PR must be updated

Resolves brave/brave-browser#9867

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:

Test Plan:

Use STR from brave/brave-browser#9867

@AlexeyBarabash AlexeyBarabash added UI CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Mar 7, 2021
@AlexeyBarabash AlexeyBarabash self-assigned this Mar 7, 2021
@AlexeyBarabash
Copy link
Contributor Author

Here is how the updated build looks, no Chromium logo during flips:

20210307_170710.mp4

@jamesmudgett is that enough, or we must to put the Brave logo?
Note, that Chromium logo has a horizontal symmetry and Brave doesn't, so with the Brave logo animation will have either begin or stop frame in upside down position.

@deeppandya
Copy link
Contributor

@AlexeyBarabash can we use vector images instead of different .pngs?

@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented Mar 7, 2021

@AlexeyBarabash can we use vector images instead of different .pngs?

@deeppandya with our organisation of patching it is hard, pls see https://github.com/brave/brave-core/compare/flip_icon_vectored?expand=1 . Also I had to remove Chromium these pngs:

src$ git status chrome/android/java/res/drawable-hdpi/logo_card_back.png chrome/android/java/res/drawable-mdpi/logo_card_back.png chrome/android/java/res/drawable-xhdpi/logo_card_back.png 
HEAD detached at 89.0.4389.72
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    chrome/android/java/res/drawable-hdpi/logo_card_back.png
	deleted:    chrome/android/java/res/drawable-mdpi/logo_card_back.png
	deleted:    chrome/android/java/res/drawable-xhdpi/logo_card_back.png

, but patches were not generated by npm run update_patches

Otherwise Chromium fails the build with error it found resources not listed on chrome_java_resources.gni.

So the best still seems to use overwrite of png.

@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review March 9, 2021 10:21
@jonathansampson
Copy link
Contributor

Why not place a Brave logo in there instead of an empty graphic?

@jamesmudgett
Copy link
Collaborator

No need for additional graphic on flip.

Copy link
Collaborator

@jamesmudgett jamesmudgett left a comment

Choose a reason for hiding this comment

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

Looks great, we don't need to keep this graphic element, adds extra bulk.

@AlexeyBarabash
Copy link
Contributor Author

Rebased to latest master, post init CI failed on audit-deps, not related to the PR.

@AlexeyBarabash AlexeyBarabash merged commit eff0512 into master Mar 19, 2021
@AlexeyBarabash AlexeyBarabash deleted the flip_icon branch March 19, 2021 18:31
@AlexeyBarabash AlexeyBarabash added this to the 1.24.x - Nightly milestone Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Chrome logo appears when flipping open tabs
4 participants