Web notifications #49
Conversation
2) implement notification toggle rough draft 3) style announcements component header
2) create notification interface
I guessed the color, but the css/html of toggle switch is implemented in ToggleSwitch.js
| // firebase doc that triggered db change event | ||
| const changedDoc = querySnapshot.docChanges()[0] | ||
|
|
||
| // don't want to notify on 'remove' + 'modified' db events |
There was a problem hiding this comment.
maybe we want to notify on 'modified' events tho 🤷♂️
There was a problem hiding this comment.
okok I know this is technically out of scope, but thoughts on adding a 'isModified' prop to the Announcement component and add a little edited @ HH:MM:SS message so people know what's been updated
There was a problem hiding this comment.
I don't think we need to use a notification when events are modified but maybe some other signifier 👍
| export const NOTIFICATION_PERMISSIONS = Object.freeze({ | ||
| GRANTED: 'granted', | ||
| DEFAULT: 'default', | ||
| DENIED: 'denied', | ||
| }) |
There was a problem hiding this comment.
was debating whether to put these variables here or in new util module notifications.js, thoughts?
There was a problem hiding this comment.
think it should be ok here for now, but if ever need to add more constants then it might be a good idea
| const notifyUser = (announcement) => { | ||
| const isRecent = new Date() - new Date(announcement.timestamp) < 5000 | ||
| if (isRecent && notifications.areEnabled()) { | ||
| notifications.trigger("New Announcement", announcement.content) | ||
| } | ||
| } |
| // firebase doc that triggered db change event | ||
| const changedDoc = querySnapshot.docChanges()[0] | ||
|
|
||
| // don't want to notify on 'remove' + 'modified' db events |
There was a problem hiding this comment.
okok I know this is technically out of scope, but thoughts on adding a 'isModified' prop to the Announcement component and add a little edited @ HH:MM:SS message so people know what's been updated
src/components/ToggleSwitch.js
Outdated
| const ToggleSwitchGraphic = styled.div` | ||
| width: 35px; | ||
| height: 30px; | ||
| background: #4F4A59; |
There was a problem hiding this comment.
I think p => p.theme.colors.foreground is this colour exactly
src/components/ToggleSwitch.js
Outdated
| text-align: right; | ||
| padding: 0 10px; | ||
| bottom: 10px; | ||
| color: rgba(0,0,0,.5); |
There was a problem hiding this comment.
is this just half opacity black? what is this supposed to do
There was a problem hiding this comment.
don't think we need this actually
| return ( | ||
| <ToggleSwitchContainer> | ||
| <label> | ||
| <Input |
There was a problem hiding this comment.
love the semantic HTML here
| export const NOTIFICATION_PERMISSIONS = Object.freeze({ | ||
| GRANTED: 'granted', | ||
| DEFAULT: 'default', | ||
| DENIED: 'denied', | ||
| }) |
There was a problem hiding this comment.
think it should be ok here for now, but if ever need to add more constants then it might be a good idea
| if (notifications.isCurrentPermission(N_PERMISSIONS.DEFAULT)) { | ||
| notifications.requestPermission(permission => { | ||
| toggleNotifications(permission === N_PERMISSIONS.GRANTED) | ||
| }) | ||
| } |
There was a problem hiding this comment.
is this asking permission to enable notifications for the first time? might be good to add a comment here
src/containers/NotificationToggle.js
Outdated
|
|
||
| useEffect(() => { | ||
| setToggled(notifications.areEnabled()) | ||
| }, []) |
There was a problem hiding this comment.
add setToggled into dependency array
2) add some comments 3) clean up toggle switch styling some more
Notificationapi related logicadded some misc comments below for stuff I wasn't sure about 👍
we are welcoming lots lots lots lots lots of feedback ! ! ! !!! !! ! !!!!!!