Show only favicons

[dlion]

Close #1657

[luixxiul]

Nice! I tried it by pulling your repo, and I noticed some points.

• Enabling this option should not be applied to bookmarks in the folder (pulldown)
• Padding between bookmarks should be narrower
• Disabling the option should display the titles again. Until you click the pull down, the titles of all bookmarks are not displayed. Currently those of the bookmarks added after disabling are displayed properly.

Thanks!

[dlion]

Thanks, I'm new to brave development so any suggestions to do this is appreciated! 😃
I will work on these points anyway

[bbondy]

any update on the above? Also please rebase the commits to clean up history a bit to squash the commits git rebase -i master. Thanks!

[dlion]

Sorry for the delay (academic reasons), currently I'm working on it 😄

• Not applied to bookmarks in the folder
• Padding between bookmakrs should be narrower
• Render the bar when option change

@luixxiul How much padding should I set ? 5px to 2px ?

[luixxiul]

@dlion, I like the margin and padding of .bookmarksToolbar .bookmarkToolbarButton and the margin of .bookmarksToolbar .bookmarkToolbarButton .bookmarkFavicon is set to 0px like this:

Also as @bbondy said please do not forget to run git rebase -i master after fetching, squash the commits to simplify the commit history and reword the committing message with including auto-closing keyword to close the issue like Close #1657, and push -f.

[dlion]

Currently I don't understand how to render the bookmarks bar after changing the option, any suggestions ? @luixxiul
And yes no problem, after all I will squash my commits. Thanks!

[bsclifton]

@dlion I downloaded your code locally, ran it, and see the issue

When the preferences change, the items aren't recreated. I created a handler for this situation (my use case was having the menu show/auto hide, which requires changes to the BrowserWindow object).

Please see here:
https://github.com/brave/browser-laptop/blob/master/js/stores/appStore.js#L250

[bsclifton]

Here's an example of where you can make the change:

function handleChangeSettingAction (settingKey, settingValue) {
  switch (settingKey) {
    case settings.AUTO_HIDE_MENU:
      BrowserWindow.getAllWindows().forEach(function (wnd) {
        wnd.setAutoHideMenuBar(settingValue)
        wnd.setMenuBarVisibility(!settingValue)
      })
      break
    case settings.SHOW_BOOKMARKS_TOOLBAR_ONLY_FAVICON:
      // TODO: recreate the BookmarksToolbar instance here, so it re-renders with the change
      break
    default:
  }
}

edit:
Although, I'm curious why it re-renders fine when you toggle showing the Bookmarks Toolbar... but not in this situation.

[bbondy]

@bsclifton in your situation you had to explicitly do it because state changes aren't propagated into electron actions. But in this case, and anything in React rendered UI, if the setting changes, it calls render on the component so no explicit action is necessary. You just have to handle the re-rendering. So trying to re-crete a BookmarksToolbar instance or any other React component isn't right.

[dlion]

So if the @bsclifton solution isn't right, how can I re-render the bookmarksToolbar ? @bbondy

[bbondy]

If you change state then render will be called on BookmarkToolbar. You just need to define how to display the UI based on the state there.

[bsclifton]

How's it going @dlion? Do you need help checking out this last item?

[dlion]

Sure @bsclifton , I didn't figure out how to change the state of the bookmarksToolbar element in the appStore file. (I'm a newbie about React and Electron but I want to learn)

[bsclifton]

Here we go @dlion, got it fixed and working 😄
https://github.com/bsclifton/browser-laptop/commit/77c1de72bdfeea207c52f91cc4246c266622e502

I'm glad this came up because I also had the chance to clean up the code I did initially for the favicons

Why does this work?

main.js creates the BookmarkToolbar control. My change was to bind the settings to that control directly. Then in the child controls, it only checks it's property state.

React triggers a re-render when any properties it's observing (used while rendering) change. So now changing the settings causes the full re-render.

[bsclifton]

@dlion you might want to rebase against master... then you should be able to cherry pick my commit (please let me know if you have trouble doing that)

Worst case scenario, you can just manually apply my changes in your branch 😄

[dlion]

Thanks @bsclifton, I learned a lot!
I've done everything.

[luixxiul]

@dlion and @bsclifton, nice work!

I tried it and got this:

In the mode there should be padding-left: 10px; added (6px is based on .tabs, 4px on .tabIcon) to .showOnlyFavicon.

cf: ffe1669#diff-474538b5e876b82157a7e1b6228743e8R21

[dlion]

@luixxiul I added the padding-left: 10px; to .showOnlyFavicon and this is the result:

[luixxiul]

Thanks, I'm working on it too, wait for a moment.

[luixxiul]

OK, it's like this:

How do you think?

Feel free to cherry pick and squash it :-)

[dlion]

@luixxiul great, committed and pushed!

[luixxiul]

@dlion in the commit message you're trying to close the PR itself, but maybe you're closing #1657, aren't you?

[dlion]

ops, you are right, corrected! ⏩

[bsclifton]

tested it out, lgtm! Well done, @dlion 😄