zluca / Sidebar

WebExtension that imprement Sidebar into browser.
GNU General Public License v3.0
31 stars 9 forks source link

Suggestion: Use small/medium/large style options for font size #32

Closed Su-Well closed 5 years ago

Su-Well commented 5 years ago

The current way of setting font size based on pixel has a few drawbacks:

The last point applies to me, so I will try to code this myself, and I will of course share the code here.

Also let me say that I love this extension, and your code is a work of art @zluca. Thank you so much for your hard work, and especially for making it open source ❤️

Su-Well commented 5 years ago

Solved (for me at least) in pull request #33

Su-Well commented 5 years ago

looks like it might have messed up fonts a little for the custom homepage? I don't know how it looked before.

zluca commented 5 years ago

@Su-Well Plz read the review I made to your PR.

Su-Well commented 5 years ago

Continuing the discussion here instead of the PR. I've been giving this some thought, and I just pushed another commit that correctly adjusts both for changes to devicePixelRatio and user moving window to another monitor.

I was gonna write that code up as a suggestion, but I figured it was easier to just do it rather than explain it (see last commit in the PR). This solution solves every case I can think of, with one exception: when a user opens a page that is already zoomed, the sidebar text will also be zoomed by the same amount. In other words, it adjusts for changes to browser zoom, but not initial zoom state for a website.

The reason for this is of course that we can't know the initial zoom state. We can always make a guess based on devicePixelRatio, but as I experienced, that guess won't work for everyone.

If you'd like to go for a solution like this one, there are a few possible workarounds to that problem, like a "reset font-size" (or zoom out/in text) button and/or saving the zoom state (devicePixelRatio) for a domain across sessions.

Su-Well commented 5 years ago

never mind! I didn't consider that extensions have access to more info than websites 😅 Please review the last commit in the PR, issue should be completely fixed now.

Su-Well commented 5 years ago

actually it looks like there is a bug when switching tabs. I don't have time to look into it right now, but I wanted to let you know so you don't merge it before the bug is resolved

zluca commented 5 years ago

@Su-Well It's ok. Take your time. =)

Olfried2 commented 5 years ago

@zluca , Thanks for this - I needed it and enjoy it with the limitations Su-Well metioned. I'm using the linux Firefox and so the sidebar is huge. @Su-Well Maybe I can help testing your enhancements on Linux

zluca commented 5 years ago

@Olfried2 I'm Arch user. :smiley:

zluca commented 5 years ago

@Su-Well Check out "font-size" branch in main repository. I'm curious how it work at your config with 2 monitors.

Su-Well commented 5 years ago

Sorry about the late reply. I'm getting an error on the current font-size branch: image and image

Off-topic but: How do you know someone's an Arch user?

You don't have to, because they will tell you 😄

(I'm also an Arch user)

zluca commented 5 years ago

Oh shit, im only tested it at startpage. It looks like browser.tabs unavailable in normal tab. So, i will make a patch tomorrow or day after to move this code to background script.

zluca commented 5 years ago

@Su-Well It's ready for testing.

Su-Well commented 5 years ago

works much better 😃 There are still two minor issues. I don't think they are significant enough to need fixing, but I'll post them here just for completeness.

First, the font size is kind of blurry, and has some artifacts on my 1440p monitor: image

Second, when zooming a page, the sidebar text will adjust for the zoom event before the zoom event actually happens, causing a slight stutter effect.

zluca commented 5 years ago

the font size is kind of blurry

My guess it's because font size is float, commit b87547c maybe fix this.

Second, when zooming a page, the sidebar text will adjust for the zoom event before the zoom event actually happens, causing a slight stutter effect.

Zooming is now handling in background page so some timing issues are normal. Anyway users don't zoom every second.

zluca commented 5 years ago

Closed in 0.6.0 release.