zulip-desktop
zulip-desktop copied to clipboard
Feature/dnd duration dropdown
Added drop down menu for users to enable Do Not Disturb (DND) mode for a selectable duration, after which the app will automatically restore the original notification settings.
First had the logic in the dnd-util.ts. Got this working, but wanted to handle the dnd-toggle request logic in app/main/index.ts to better follow Zulip's contribuiting guide.
We listen for our LeftSidebarEvents() & dndButton EventListener() aka our renderer to send a message to our ipcMain.on() in app/main/index.ts. This is where we now handle the logic and send message back to the renderer with the new parameters.
Had some small UI issues, with modal overlays covering things. Was able to fix. Tested functionality with numerous test cases, logging results to console. Haven't found an edge case that breaks the code.
Feel free to comment on style or content within the dropdown.
This is my first open source contribution. Please bare with me. Thanks!
Fixes: (https://github.com/zulip/zulip-desktop/issues/561)
Tooling tips: https://zulip.readthedocs.io/en/latest/tutorials/screenshot-and-gif-software.html -->
Screenshots and screen captures:
Platforms this PR was tested on:
- [x] Windows
- [ ] macOS
- [ ] Linux (specify distro)
Self-review checklist
- [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
- [x] Explains differences from previous plans (e.g., issue description).
- [x] Highlights technical choices and bugs encountered.
- [x] Calls out remaining decisions and concerns.
- [x] Automated tests verify logic where appropriate.
Individual commits are ready for review (see commit discipline).
- [x] Each commit is a coherent idea.
- [x] Commit message(s) explain reasoning and motivation for changes.
Completed manual review and testing of the following:
- [x] Visual appearance of the changes.
- [x] Responsiveness and internationalization.
- [x] Strings and tooltips.
- [x] End-to-end functionality of buttons, interactions and flows.
- [x] Corner cases, error conditions, and easily imagined bugs.
@lolostheman thanks for the PR! Can you clean up the commit history to make it easier to review? The commits here seem to have a lot of churn where one commit is fixing up issues in the previous one. Especially code moves are best done in commits that don't make any changes other than moving the code and adjusting imports.
Check out our GitHub guide and commit guidelines for advice on how to do a nicely reviewable PR.
If the app exits (or the computer crashes, etc.) before the timer expires, this will make the temporary setting permanent. I don’t think that’s what a user would expect.
If the app exits (or the computer crashes, etc.) before the timer expires, this will make the temporary setting permanent. I don’t think that’s what a user would expect.
Yes, this would indeed happen. if the app crashes before the setTimeout callback fires. I'll work on a fix. Probably by storing the duration in the config file, and then we can check if the duration has expired on startup.
@lolostheman Is this PR ready for the next round of review? It's helpful to post a comment explicitly noting this if so.
@lolostheman Is this PR ready for the next round of review? It's helpful to post a comment explicitly noting this if so.
@alya Yes, the PR is ready for another review.
@alya are their any other steps I should take to try to push this PR to a reviewer? Or is it just a waiting game?
@lolostheman sorry for the slow reply. In general, PRs that look like we can predictably merge them quickly tend to get faster reviews. Leaving stale testing code and having readily visible issues (like no translation tags) generally result in PRs being delayed until we have more time for them.
I don't personally work in this project much. I expect @shubham-padia will be reviewing this eventually as he's ramping up on the codebase. You're certainly welcome to work on another issue in the project if you'd like! I am hopeful that we'll be able to be more responsive to new-to-OSS PRs in the coming weeks than we have been historically.
I noticed this repo wasn't very active before I started to even work on this issue. So that being said I was well aware of the potential slow PR reviews. Thanks for taking the time to look at my PR once again. I rebased and added the translation tags. Let me know if there is any thing else. Take your time at reviewing, no rush. @timabbott
Heads up @lolostheman, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.