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

History uses Chromium bookmark icon for bookmark entries #6013

Open
srirambv opened this issue Sep 13, 2019 · 8 comments
Open

History uses Chromium bookmark icon for bookmark entries #6013

srirambv opened this issue Sep 13, 2019 · 8 comments

Comments

@srirambv
Copy link
Collaborator

@srirambv srirambv commented Sep 13, 2019

Description

History uses Chromium bookmark icon for bookmark entries

Steps to Reproduce

  1. Visit any site and bookmark
  2. Open Hisory
  3. Shows Chromium bookmark icon instead of Brave bookmark icon

Actual result:

image

Expected result:

Use Brave branding icons everywhere

Reproduces how often:

Easy

Brave version (brave://version info)

Brave 0.68.132 Chromium: 76.0.3809.132 (Official Build) (64-bit)
Revision fd1acc410994a7a68ac25bc77513d443f3130860-refs/branch-heads/3809@{#1035}
OS Linux

Version/Channel Information:

  • Can you reproduce this issue with the current release? Yes
  • Can you reproduce this issue with the beta channel? Yes
  • Can you reproduce this issue with the dev channel? Yes
  • Can you reproduce this issue with the nightly channel? Yes

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields?
  • Does the issue resolve itself when disabling Brave Rewards?
  • Is the issue reproducible on the latest version of Chrome?

Miscellaneous Information:

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Sep 13, 2019

@srirambv can you be more specific about what is wrong? When I look at the picture shown, it looks good to me (shows picture of Brave icon).

@srirambv
Copy link
Collaborator Author

@srirambv srirambv commented Oct 15, 2019

@bsclifton the icon on right is chromium star we replaced the bookmark icon with our custom icon which should be used.

@rebron rebron added this to P4 backlog in Front End Oct 21, 2019
@karenkliu
Copy link
Collaborator

@karenkliu karenkliu commented Dec 10, 2019

@srirambv I don't see the star? This is what my bookmarks looks like:
Screen Shot 2019-12-10 at 11 24 09 AM
Version 1.4.16 Chromium: 79.0.3945.74 (Official Build) nightly (64-bit)

The bigger issue for me is we should be using our own bookmarks folder icon:
icon-folder@1 5x

It replaces the Chrome bookmarks folder icon in the browser toolbar and on chrome://bookmarks/
example

Please use our Brave folder icon:
bookmark folder icon.zip
(Maybe this should be a separate issue though)

@srirambv
Copy link
Collaborator Author

@srirambv srirambv commented Dec 10, 2019

@karenkliu need to check the entry in history(brave://history). If the site is bookmarked it shows the star next to it as shown in issue.

@rebron
Copy link
Collaborator

@rebron rebron commented Dec 10, 2019

Screen Shot 2019-12-11 at 5 00 05 AM

another example but in dark mode
@karenkliu
Copy link
Collaborator

@karenkliu karenkliu commented Dec 13, 2019

oh got it - okay, you're saying it should look like this?
bookmarked site

Here you go:
fill-bookmarked.zip
Let's make it 16x16 large.

@darrylbutcher
Copy link

@darrylbutcher darrylbutcher commented Dec 23, 2019

Looks like the fix would be creating a patch on this file in the chromium source. If theres some documentation on how you create your chromium patches/how to validate and test them I can give it a shot

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Mar 4, 2020

Hi @darrylbutcher - our wiki has some good information about getting setup:
https://github.com/brave/brave-browser/wiki

We do have a specific section about patching too:
https://github.com/brave/brave-browser/wiki/Patching-Chromium

This might be as easy as creating a new file under chromium_src (matching directory structure of existing icon)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Front End
  
P4 backlog
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.