Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add font-awesome folder icon for folders on bookmarks toolbar and menus #1918

Merged
merged 1 commit into from
May 28, 2016
Merged

Add font-awesome folder icon for folders on bookmarks toolbar and menus #1918

merged 1 commit into from
May 28, 2016

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 25, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Ran git rebase -i to squash commits if needed.

Fix #1469

Image 1:

Please make sure that @bookmarksIconSize: 16px; is based on const iconSize = 16 (here).

I like the white icon better than fa-folder, which is a black icon.
http://fontawesome.io/icon/folder/

@luixxiul luixxiul added the design A design change, especially one which needs input from the design team. label May 25, 2016
@@ -157,6 +157,11 @@ class ContextMenuItem extends ImmutableComponent {
style={iconStyle}></span>
: null
}
{
this.hasSubmenu
? <span className='submenuIndicator contextMenuIcon bookmarkIcon fa fa-folder-o' style={iconStyle} />
Copy link
Member

Choose a reason for hiding this comment

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

The word bookmark shouldn't be in this file since it's a general context menu file.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to modify js/contextMenu.js and look in bookmarkItemsInit. That's where icons are set. If you find it is a folder you can add a new property like faIcon and then if it is specified inside js/compnents/contextMenu.js you can use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I will

@luixxiul luixxiul changed the title Add font-awesome folder icon for folders on bookmarks toolbar and menus Add font-awesome folder icon for folders on bookmarks toolbar May 25, 2016
@luixxiul
Copy link
Contributor Author

luixxiul commented May 25, 2016

I'm trying to add bookmark icons in submenu, but can't :/

@bsclifton
Copy link
Member

You were really close! Here's what I did that made it work:
https://github.com/bsclifton/browser-laptop/commit/98e06cfcfe6d08f43d6bcfab44fb07f5204517f6

Feel free to either make the commit yourself or add my remote and then cherry pick this hash (I can help if you don't know how to do that)

😄

@luixxiul
Copy link
Contributor Author

Thanks! It was a great help :-)

@luixxiul luixxiul changed the title Add font-awesome folder icon for folders on bookmarks toolbar Add font-awesome folder icon for folders on bookmarks toolbar and menus May 28, 2016
@luixxiul
Copy link
Contributor Author

OK I think it's ready.

@bsclifton
Copy link
Member

lgtm! 👍

@bsclifton bsclifton merged commit 3674390 into brave:master May 28, 2016
@luixxiul luixxiul added this to the 0.10.1dev milestone May 29, 2016
@luixxiul luixxiul deleted the bookmarkicon branch May 29, 2016 05:21
@luixxiul luixxiul restored the bookmarkicon branch May 29, 2016 17:42
@luixxiul
Copy link
Contributor Author

luixxiul commented May 29, 2016

I think .bookmarkToolbar should be always 28px, in order to avoid this:

clipboard01

Should I send another PR to fix this?

@bsclifton
Copy link
Member

Interesting.. what platform are you on? Here's how it looks for me (Windows).
2016-05-29 2

Do the fonts get offset differently because you have unicode characters in the bookmarks toolbar?

@luixxiul
Copy link
Contributor Author

luixxiul commented May 29, 2016

It seems that the difference of padding settings causes it. If you set padding: 0px 10px in .bookmarksToolbar they are aligned well.

Either hiding the folder icon in the hide favicon mode or changing the folder icon size would also work.

Edit: this was fixed with 4c981f5, e142c53, and #1973.

@luixxiul luixxiul deleted the bookmarkicon branch May 30, 2016 04:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants