Add setting to skip desktop nofifications

dan1d commented

2018-07-31 02:17:49 +02:00

(Migrated from github.com)

Closes issue #1788
It persist the setting on localstorage, defaults to true(notifications enabled), I've created a selector to check if the notification should be run.
Example:
if(selectDesktopNotificationsEnabled(state)) thenNotify()

The setting is present here, let me know If I should change the text/section of the checkbox.

Closes issue #1788 It persist the setting on localstorage, defaults to true(notifications enabled), I've created a selector to check if the notification should be run. Example: `if(selectDesktopNotificationsEnabled(state)) thenNotify()` The setting is present here, let me know If I should change the text/section of the checkbox. ![screenshot from 2018-07-30 21-10-53](https://user-images.githubusercontent.com/3293616/43430176-00ac2d0a-943e-11e8-910c-abd1ec31a5d8.png)

👍 1

kauffj commented

2018-07-31 23:00:08 +02:00

(Migrated from github.com)

This should not be clustered until the Download Directory. I'd suggest making a new section with a header of "Notifications". The label for the checkbox can be "Notify me when a download completes" or just "On Download Completion", whichever looks better.
@seanyesmunt is the checkbox used on the file show page intended to be the new checkbox used everywhere? If so, it should be used here (and really, replace all checkboxes). Mixed styles for the same element are a big design inconsistency.

1) This should not be clustered until the Download Directory. I'd suggest making a new section with a header of "Notifications". The label for the checkbox can be "Notify me when a download completes" or just "On Download Completion", whichever looks better. 2) @seanyesmunt is the checkbox used on the file show page intended to be the new checkbox used everywhere? If so, it should be used here (and really, replace all checkboxes). Mixed styles for the same element are a big design inconsistency.

dan1d commented

2018-08-01 00:30:00 +02:00

(Migrated from github.com)

@kauffj on point 1, I've made the code to enable/disable all kind of desktop notifications, not just when it finishes downloading, should I add code to cover only this case: "Notify me when a download completes" ?
If so I should change the property to be something like this: finishedDownloadNotificationEnabled rather than the global desktopNotificationsEnabled

@kauffj on point 1, I've made the code to enable/disable all kind of desktop notifications, not just when it finishes downloading, should I add code to cover only this case: "Notify me when a download completes" ? If so I should change the property to be something like this: `finishedDownloadNotificationEnabled` rather than the global `desktopNotificationsEnabled`

dan1d commented

2018-08-01 00:41:45 +02:00

(Migrated from github.com)

Updated checkbox to toggle style on settings page.

Updated checkbox to toggle style on settings page. ![screenshot from 2018-07-31 19-40-44](https://user-images.githubusercontent.com/3293616/43491111-a70665ba-94f9-11e8-9fa7-0e51f7108063.png)

neb-b commented

2018-08-01 00:45:48 +02:00

(Migrated from github.com)

Yes the plan is to add that to all checkboxes. @dan1d I would check to make sure the description is still lined up properly on smaller screens when the description text has to wrap to the next line.

tzarebczan commented

2018-08-01 16:54:56 +02:00

(Migrated from github.com)

The notifications include more than just download complete - we also do notifications for subscriptions when new content is posted. And we'll probably add more (think there's an issue for adding a published complete notification).

IMO, it should be called something along the lines of OS/System Notifications so as not to be confused with the in-app notification system (when it's added).

The notifications include more than just download complete - we also do notifications for subscriptions when new content is posted. And we'll probably add more (think there's an issue for adding a published complete notification). IMO, it should be called something along the lines of OS/System Notifications so as not to be confused with the in-app notification system (when it's added).

neb-b commented

2018-08-01 17:26:07 +02:00

(Migrated from github.com)

Yeah I think for the first step we will just add a blanket mute for all notifications. I like @tzarebczan's idea of calling them OS Notifications

Maybe the description should be Hide OS notifications

Yeah I think for the first step we will just add a blanket mute for all notifications. I like @tzarebczan's idea of calling them `OS Notifications` Maybe the description should be `Hide OS notifications`

dan1d commented

2018-08-01 20:31:17 +02:00

(Migrated from github.com)

I've changed the variable names to osNotificationsEnabled and the selector to selectOsNotificationEnabled.
@seanyesmunt I've changed the word from Hide OS nofitications to Show OS notifications to keep it in sync with Show NSFW content checkbox. Is that ok?

The check sign has a weird behavior on smaller screen.

I've changed the variable names to `osNotificationsEnabled` and the selector to `selectOsNotificationEnabled`. @seanyesmunt I've changed the word from `Hide OS nofitications` to `Show OS notifications` to keep it in sync with `Show NSFW content` checkbox. Is that ok? The check sign has a weird behavior on smaller screen. ![screenshot from 2018-08-01 15-29-05](https://user-images.githubusercontent.com/3293616/43541002-a8978476-959f-11e8-86ee-b3b7bab287d7.png)

kauffj commented

2018-08-01 20:53:11 +02:00

(Migrated from github.com)

@dan1d looks great! Can you label it "Show Desktop Notifications"?

neb-b commented

2018-08-01 21:26:27 +02:00

(Migrated from github.com)

Nice. You may need to make some change to _toggle.scss. Not sure what would be causing that.

Nice. You may need to make some change to `_toggle.scss`. Not sure what would be causing that.

dan1d commented

2018-08-01 21:49:17 +02:00

(Migrated from github.com)

Thanks @seanyesmunt,
I've Added margin-top: auto; margin-bottom: auto; to .react-toggle class and it adjust automatically to the center of the parent box. Which fix the issue. Let me know if I should change that part.

Or do you prefer the toggle to stay at the top? regardless of the parent size ?

Thanks @seanyesmunt, I've Added `margin-top: auto; margin-bottom: auto;` to `.react-toggle` class and it adjust automatically to the center of the parent box. Which fix the issue. Let me know if I should change that part. ![screenshot from 2018-08-01 16-47-43](https://user-images.githubusercontent.com/3293616/43544865-a223b2d0-95aa-11e8-83a3-15f84a9422f5.png) Or do you prefer the toggle to stay at the top? regardless of the parent size ?

neb-b commented

2018-08-01 23:50:11 +02:00

(Migrated from github.com)

I think that fix is fine (or a similar one), but in my opinion the toggle should be at the top. In-line with the first line of the description.

dan1d commented

2018-08-02 00:38:53 +02:00

(Migrated from github.com)

Done!

Done! ![screenshot from 2018-08-01 19-38-32](https://user-images.githubusercontent.com/3293616/43552824-82ca2186-95c2-11e8-9e0d-0a2201237dd0.png)

neb-b commented

2018-08-03 16:49:16 +02:00

(Migrated from github.com)

@dan1d Just tested this and it looks great! One thing I forgot to say is that we should probably just get rid of the useToggle prop and always use this toggle. That was only when we only used the toggle in one place. Once that is done and a changelog entry is added this can be merged.

Nice work! 🙂

@dan1d Just tested this and it looks great! One thing I forgot to say is that we should probably just get rid of the `useToggle` prop and always use this toggle. That was only when we only used the toggle in one place. Once that is done and a changelog entry is added this can be merged. Nice work! 🙂

👍 1

dan1d commented

2018-08-06 00:37:14 +02:00

(Migrated from github.com)

@seanyesmunt Thanks for the review. Removed the useToggle prop and added the changelog entry.

Add setting to skip desktop nofifications #1834