Skip to content

Add feature to enable Time based DND Mode #896

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add feature to enable Time based DND Mode #896

wants to merge 2 commits into from

Conversation

manavmehta
Copy link
Collaborator

@manavmehta manavmehta commented Mar 10, 2020 •
edited
Loading

What's this PR do?
Fixes: #561
User can now select time for which DND will be enabled.
Toast notification (4 sec) will be shown for clock time when DND would go off and also when it actually goes off.

Screenshots?
Screenshot 2020-03-11 at 12 59 19 AM
Screenshot 2020-03-11 at 12 58 29 AM

You have tested this PR on:
[✓ ] macOS Catalina 10.15.4

@manavmehta
Copy link
Collaborator Author

@timabbott @akashnimare @abhigyank
Can you please review and suggest !?!

@abhigyank abhigyank requested a review from akashnimare March 12, 2020 16:07
abhigyank
abhigyank suggested changes Mar 12, 2020
View reviewed changes
@abhigyank
Copy link
Collaborator

Left some comments. I like how it works smoothly for both the themes.

@manavmehta manavmehta changed the title [WIP] Add feature to enable Time based DND Mode Add feature to enable Time based DND Mode Mar 12, 2020
@manavmehta
Copy link
Collaborator Author

@abhigyank Thanks. I have implemented your suggestions and toast is also done now. So it's a completed PR from my end.
@akashnimare @timabbott Please review. Suggestions welcomed :)

abhigyank
abhigyank reviewed Mar 13, 2020
View reviewed changes
@abhigyank
Copy link
Collaborator

The working is fine, it is working as expected for me. The overall logic implementation looks correct to me.
@akashnimare Can you do a code quality check? It is then good to merge.

akashnimare
akashnimare reviewed Mar 13, 2020
View reviewed changes
@akashnimare
Copy link
Member

@manavmehta can you refactor the PR a bit (there are some places where you can improve the code quality + add comments etc)?

manavmehta
manavmehta commented Mar 14, 2020
View reviewed changes
Copy link
Collaborator Author

@manavmehta manavmehta left a comment

Choose a reason for hiding this comment

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

@akashnimare any specifics for refactoring - any function

@manavmehta can you refactor the PR a bit (there are some places where you can improve the code quality + add comments etc)?

Any specific block for refactoring and improving code quality? I can surely implement if you mention.

@akashnimare
Copy link
Member

I have sent you a few things which you can improve in the PM.

@manavmehta
Copy link
Collaborator Author

@akashnimare @abhigyank implemented. Please review.
Also if you may see if map would be better than object in makeView()

@akashnimare
Copy link
Member

This looks better I think. @andersk quick review?

@abhigyank
Copy link
Collaborator

Working LGTM. Refactoring has improved the code. Can be merged after @andersk does a review.

@manavmehta
Copy link
Collaborator Author

@andersk ping

@manavmehta manavmehta mentioned this pull request Apr 1, 2020
Open
@abhigyank
Copy link
Collaborator

@manavmehta Rebase this once you're free. We can merge this @akashnimare .

@manavmehta
Copy link
Collaborator Author

@abhigyank Rebased. Tested again on macOS.

@manavmehta manavmehta force-pushed the DND branch 2 times, most recently from 163fb43 to 64ad096 Compare June 7, 2020 17:11
akashnimare
akashnimare reviewed Jun 13, 2020
View reviewed changes
akashnimare
akashnimare reviewed Jun 13, 2020
View reviewed changes
@manavmehta
dnd: Add feature to enable time based DND Mode
20e4f12 
Create a menu to allow selecting time for which DND will be enabled.
DND will go off at the scheduled time and a toast will be shown, both while setting the DND, thus conveying the DND Off time and again at the time when DND goes off.

'Until I resume' option is time independent, trivially.

Fixes zulip#561
abhigyank
abhigyank reviewed Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@abhigyank abhigyank left a comment

Choose a reason for hiding this comment

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

dndswitchOff would probably be better named as dndEndTime ?

abhigyank
abhigyank reviewed Jun 15, 2020
View reviewed changes
abhigyank
abhigyank reviewed Jun 15, 2020
View reviewed changes
@manavmehta
Refactoring:
5b6a9cc 
* Better documentation
* Use better methods like `append` instead of `innerHTML`
* Better function names and parameters
@akashnimare akashnimare mentioned this pull request Jun 25, 2020
Closed
@zulipbot
Copy link
Member

Heads up @manavmehta, we just merged some commits that conflict with the changes your 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/master branch and resolve your pull request's merge conflicts accordingly.

Base automatically changed from master to main January 22, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhigyank abhigyank Awaiting requested review from abhigyank

@akashnimare akashnimare Awaiting requested review from akashnimare

Assignees
No one assigned
Labels
has conflicts size: XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a feature to configure DND mode
4 participants
@manavmehta @abhigyank @akashnimare @zulipbot