Skip to content
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

New setting: Close on quit #358

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

leaftail1880
Copy link
Contributor

изображение

Feature request: https://discord.com/channels/395931304870281216/395933121981317120/1280074401701691453

Also slightly reworked global.d.ts to properly add types to global

@leaftail1880 leaftail1880 changed the base branch from master to develop September 2, 2024 09:37
Copy link
Owner

@mvdicarlo mvdicarlo left a comment

Choose a reason for hiding this comment

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

Would like to hear your opinion on the listener comment.

nits are non-blockers.

});
notification.show();
}, 750);
if (!global.settingsDB.getState().quitOnClose)
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like the 'closed' event listener's instantiation shouldn't be affected by the setting. It would be better to just stick the 'if' inside and do an app quit when true. This would support better behavior imo and would allow the app to instantly take any updates to the setting into account when the user then closes the window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats right! I will update that

ui/src/views/settings/SettingsView.tsx Outdated Show resolved Hide resolved
/>
</Form.Item>
<p>
To use scheduling feature, this settings <strong>must be</strong> disabled.
Copy link
Owner

Choose a reason for hiding this comment

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

nit: technically untrue as the app could still post as long as the window was minimized, although I understand the purpose. Probably fine to keep as is for now or just suggest that it is recommended to disable when using scheduled posts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@leaftail1880
Copy link
Contributor Author

@mvdicarlo All done! Thanks for you review

@mvdicarlo mvdicarlo merged commit c238e65 into mvdicarlo:develop Sep 6, 2024
@leaftail1880 leaftail1880 deleted the feat-close-on-quit branch September 7, 2024 17:52
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.

2 participants