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

Add setting to skip desktop nofifications #1834

Merged
merged 8 commits into from Aug 6, 2018

Conversation

Projects
None yet
5 participants
@dan1d
Copy link
Contributor

dan1d commented Jul 31, 2018

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

Add setting to skip desktop nofifications, persist the setting on loc…
…alstorage, defaults to true(notifications enabled).
@kauffj

This comment has been minimized.

Copy link
Member

kauffj commented Jul 31, 2018

  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

This comment has been minimized.

Copy link
Contributor Author

dan1d commented Jul 31, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

dan1d commented Jul 31, 2018

Updated checkbox to toggle style on settings page.
screenshot from 2018-07-31 19-40-44

Update checkbox style to be toggable.
	- Add Notifications to be on his own section.
@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Jul 31, 2018

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

This comment has been minimized.

Copy link
Member

tzarebczan commented Aug 1, 2018

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).

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Aug 1, 2018

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

This comment has been minimized.

Copy link
Contributor Author

dan1d commented Aug 1, 2018

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

@kauffj

This comment has been minimized.

Copy link
Member

kauffj commented Aug 1, 2018

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

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Aug 1, 2018

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

@dan1d

This comment has been minimized.

Copy link
Contributor Author

dan1d commented Aug 1, 2018

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

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

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Aug 1, 2018

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

This comment has been minimized.

Copy link
Contributor Author

dan1d commented Aug 1, 2018

Done!
screenshot from 2018-08-01 19-38-32

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Aug 3, 2018

@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! 🙂

Allways use toggle on checkbox input.
	- Add changelog entry for PR 1834.
@dan1d

This comment has been minimized.

Copy link
Contributor Author

dan1d commented Aug 5, 2018

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

@seanyesmunt seanyesmunt merged commit 3b390ff into lbryio:master Aug 6, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.