refactor: button component

[afonsojramos]

This PR now aims to refactor the button component to make it more modular and reusable.

Work towards #1312.

Let me know what do you think! 🧐

[setchy]

Looks really nice @afonsojramos - just played around with it locally.

A few small suggestions

• should the labels include a % suffix?
• the 75% | seems to have an off-by-one-pixel.

Really good stuff!

[setchy]

Was also just thinking of what this might look like using our existing RadioGroup component (used by Theme and Group By)

[afonsojramos]

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

[afonsojramos]

Should be good now @setchy

[afonsojramos]

@adufr does this serve your goals? Or do you need extra zoom to either side?

[setchy]

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 + =?

[afonsojramos]

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.

[afonsojramos]

Managed to find a way to do it 🤓

[afonsojramos]

Adds a bit of extra code though

[afonsojramos]

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?

[adufr]

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..?

[adufr]

Also, a while ago I introduced this related change: #1035

I'll let you check but it might not be required anymore, idk

[afonsojramos]

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

[setchy]

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.

[afonsojramos]

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.

[setchy]

You made me think? Should we only allow zoom out?

[setchy]

I noticed some odd slider behavior under the following conditions:

• click + hold the slider and move left or right. seems to glitch a fair bit for me.
• 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.

[adufr]

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:

gitify/src/electron/main.js

Lines 67 to 68 in c86e7fd

	// Force the window to retrieve its previous zoom factor
	mb.window.webContents.setZoomFactor(mb.window.webContents.getZoomFactor());

[afonsojramos]

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 ✅

[setchy]

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:

• use only fixed zoom increments: match those used in Chrome (100%, 90%, 80%, 75%, 66%, 50%, 33%, 25%)
• fix the slider to these increments, or, use a different input component (ie: dropdow)
• prevent zoom above 100% or below 25%
• reliably load the zoom level on app start

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

[afonsojramos]

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!

[afonsojramos]

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 😓

[setchy]

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 🥹

[afonsojramos]

@setchy updated this PR to be a refactor of the button component. Will do a stacked PR to add in the controls for the zoom.