Skip to content

feat: resizable window #749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

adufr
Copy link
Contributor

@adufr adufr commented Feb 9, 2024

Related issues

Context

Make the Gitify window resizable

Discussion

I decided to make it so that it's resizable by default because IMO it's better to not have too many settings.

I used localStorage to persist the size of the window across restarts (note if you're testing it in dev, make sure you properly quit gitify using the close button instead of cmd+c the pnpm start process; otherwise the size sometimes doesn't get saved across restarts (idk why))

I had to make a small hack (menubarApp.window.webContents.executeJavaScript) to get access to the localStorage in the main.js file because it was undefined in menubarApp.window; if you have a cleaner way to do this please let me know!

@@ -52,7 +52,30 @@ const menubarApp = menubar({
});

menubarApp.on('ready', () => {
// Utility function to reposition the window so it stays in the tray center
function moveToTrayCenter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do the function definitions need to be in the ready scope? Couldn't they be at the top level scope? Would make for cleaner code that's easier to refactor.

Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

As is, I disagree with the goals of this PR. I have not seen a single menubar app that allows for resizing and for a good reason I think. Either small or big, all of them allow for resizing and are always the same size. If we opt for going the depending on the screen size I suppose might consider it a bit more, but even so, I am not so sure.

@adufr
Copy link
Contributor Author

adufr commented Feb 13, 2024

@afonsojramos should I spend some time tinkering something that depends on the screen size or should we just close this PR and the related issues?

TBH I tried doing something because I saw 2 open issues about this and I also felt like the window was a bit too small to display multiples notifications, but I'm satisfied enough with the possibility to use cmd+ and cmd- to zoom/unzoom inside the window...

@afonsojramos
Copy link
Member

@adufr I'm for closing because at the moment it displays already displays a good number of notifications without becoming too overwhelming. Maybe we can rethink the size, but making it adjustable or dependant on windowsize seems to be a recipe for disaster 🫠 Always happy to be proven wrong though! Besides, we're open source, so if anyone really wants to, they can whip up their own version with resizable windows.

@adufr adufr deleted the feat/resizable-window branch February 14, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants