Open afonsojramos opened 5 days ago
Looks really nice @afonsojramos - just played around with it locally.
A few small suggestions
%
suffix?|
seems to have an off-by-one-pixel. Really good stuff!
Was also just thinking of what this might look like using our existing RadioGroup component (used by Theme and Group By)
Was also just thinking of what this might look like using our existing RadioGroup component (used by Theme and Group By)
Unless that radio button is 5 buttons wide (which looks bad imo 😬) I don't see it as being a good alternative here.
should the labels include a % suffix?
That's fair, I'll add that!
the 75% | seems to have an off-by-one-pixel.
🤔 I'll check
Should be good now @setchy
@adufr does this serve your goals? Or do you need extra zoom to either side?
Should be good now @setchy
Looks great @afonsojramos, thank you!
What's your thoughts on if the slider should reflect the manually set zoom level via Cmd + -
or Cmd + =
?
What's your thoughts on if the slider should reflect the manually set zoom level via Cmd + - or Cmd + =?
It already does, just not "live" because the zoom doesn't trigger a rerender of react.
Managed to find a way to do it 🤓
Adds a bit of extra code though
Potential issue is the fact that CMD+ and CMD- do jumps in 50 points increments, so I've widened the range to make it more accommodating of these jumps. Thoughts?
Just checked, I really like how the control looks and feels, but I'm used to unzoom even more than the 0% setting 😅
(I just counted and I press Cmd -
3 times after hitting the 0%
Also, the setting doesn't seem to be preserved across restarts..?
Also, a while ago I introduced this related change: https://github.com/gitify-app/gitify/pull/1035
I'll let you check but it might not be required anymore, idk
Also, the setting doesn't seem to be preserved across restarts..?
@setchy @adufr I've been trying to set that up (even with the settings, with no success), but considering that that is supposed to be working, let's just say that it is a bug to be fixed separately.
I just counted and I press Cmd - 3 times after hitting the 0%
Regarding this, I'll try to adjust the scale to meet that
Also, the setting doesn't seem to be preserved across restarts..?
@setchy @adufr I've been trying to set that up (even with the settings, with no success), but considering that that is supposed to be working, let's just say that it is a bug to be fixed separately.
I'm away from my computer ATM, which check later, but I suspect you may need to add special handling when the settings are first loaded after opening, like we do for the "open on startup" setting.
Implemented a 1.85 multiplier so this should simplify the calculations and reach your zoom level @adufr.
PS: If you're asking why a 1.85 multiplier and not the 2... Because a 2x multiplier would make the 150% go out of bound a bit in the 150% zoom.
Edit: Nevermind, I want the 2x multiplier, so I reduced the max zoom to 120%. I doubt anyone is going to zoom in.
You made me think? Should we only allow zoom out?
I noticed some odd slider behavior under the following conditions:
The zoom out now meets my needs 👍 But yeah I don't really see any use-case where zooming-in would be useful
click + hold the slider and move left or right. seems to glitch a fair bit for me.
It glitches because it's resizing instantaneously so the mouse gets out of the slider 😅
clicking on the sliders uses the defined increments, while using the Cmd + / Cmd - hotkeys moves in different increments, which means you can get out of sync
Well, it's visually annoying but I deem it acceptable
but I suspect you may need to add special handling when the settings are first loaded after opening, like we do for the "open on startup" setting
Yup that's why I pointed the PR #1035, I think the following line needs to be adjusted: https://github.com/gitify-app/gitify/blob/c86e7fde8c8c1e0fd05c7be489cdb0b7be65e4c9/src/electron/main.js#L67-L68
click + hold the slider and move left or right. seems to glitch a fair bit for me.
@setchy I fixed this by making the step smaller, which was more work than expected, wasted a couple of hours on this 💀 Hope you like it more like this 🔫 This does mean that keyboard navigation now only jumps steps of 1, BUT you can just press for longer I suppose, it isn't too bad.
means you can get out of sync.
It also solves this I suppose ✅
Appreciate the time spent @afonsojramos. 🙏
I hope this is user error on my part, but, I cannot get the zoom settings to load on app restart... Sometimes it works, but shows the zoom at 100% in the UI, other times it doesnt set the zoom level at all... I was also able to get the zoom out of sync quit easily with the 1% increments.
What if to simplify the whole impl it was:
Alternatively, what if we:
I cannot get the zoom settings to load on app restart
So, is this currently working? Because I can't get it to work outside of this PR either, so it feels like out of scope for the pr itself, as it is supposedly working (just not something that I had tested before)
I was also able to get the zoom out of sync quit easily with the 1% increments
Do you mean with the mouse? That's okay I'd say!
Alternatively, what if we:
- had a Zoom label in Appearance Settings
- it had a simple zoom in, zoom out, reset zoom button combo
- displayed the current zoom level as a read only value
- had a tooltip explaining how power users can use hotkeys to perform the same action
- this is then very similar to how Chrome managed user zoom
To be honest I like this approach, but am sad to trash it all 😓
Alternatively, what if we:
- had a Zoom label in Appearance Settings
- it had a simple zoom in, zoom out, reset zoom button combo
- displayed the current zoom level as a read only value
- had a tooltip explaining how power users can use hotkeys to perform the same action
- this is then very similar to how Chrome managed user zoom
To be honest I like this approach, but am sad to trash it all 😓
I'm right there with you. The life of a developer 🥹
Closes #1312.
Let me know what do you think! 🧐