[dashboard] Implemented notification settings #3513
Conversation
There was a problem hiding this comment.
Smoooooth! 🍦
Thanks @svenefftinge! The specs included a toast component for visual feedback on every change of the notifications settings, see #3398. Do we skip this for now?
|
|
||
| function CheckBox(props: {title: string, desc: string, checked: boolean, onChange: () => void}) { | ||
| return <div className="flex mt-4"> | ||
| <input className={"focus:ring-0 mt-1 rounded-sm cursor-pointer "+(props.checked?'bg-gray-800':'')} type="checkbox" checked={props.checked} onChange={props.onChange}/> |
There was a problem hiding this comment.
nitpick: Minor checkbox style. We're going to need hover, focus, and active states for the checkboxes and many other elements but let's leave this out of the scope of this iteration.
| <input className={"focus:ring-0 mt-1 rounded-sm cursor-pointer "+(props.checked?'bg-gray-800':'')} type="checkbox" checked={props.checked} onChange={props.onChange}/> | |
| <input className={"focus:ring-0 mt-1 rounded cursor-pointer border-2 "+(props.checked?'bg-gray-800':'')} type="checkbox" checked={props.checked} onChange={props.onChange}/> |
|
|
||
| function CheckBox(props: {title: string, desc: string, checked: boolean, onChange: () => void}) { | ||
| return <div className="flex mt-4"> | ||
| <input className={"focus:ring-0 mt-1 rounded-sm cursor-pointer "+(props.checked?'bg-gray-800':'')} type="checkbox" checked={props.checked} onChange={props.onChange}/> |
There was a problem hiding this comment.
issue: The checkbox should be 24x24 but this one suffices for now. Feel free to leave this as is.
There was a problem hiding this comment.
I tried it, but it somehow looks way too large. Maybe something else is off in relation.
| return <div> | ||
| <SettingsPage title='Notifications' subtitle='Email notification preferences'> | ||
| <h3>Notifications</h3> | ||
| <SettingsPage title='Notifications' subtitle='Choose when to be notified'> |
There was a problem hiding this comment.
nitpick: Minor punctuation update to align with the rest of the pages.
| <SettingsPage title='Notifications' subtitle='Choose when to be notified'> | |
| <SettingsPage title='Notifications' subtitle='Choose when to be notified.'> |
| function CheckBox(props: {title: string, desc: string, checked: boolean, onChange: () => void}) { | ||
| return <div className="flex mt-4"> | ||
| <input className={"focus:ring-0 mt-1 rounded-sm cursor-pointer "+(props.checked?'bg-gray-800':'')} type="checkbox" checked={props.checked} onChange={props.onChange}/> | ||
| <div className="flex flex-col ml-2"> |
There was a problem hiding this comment.
praise: Thanks for getting the margin in here!
| <SettingsPage title='Notifications' subtitle='Email notification preferences'> | ||
| <h3>Notifications</h3> | ||
| <SettingsPage title='Notifications' subtitle='Choose when to be notified'> | ||
| <h3>Email Notification Preferences</h3> |
There was a problem hiding this comment.
| <h3>Email Notification Preferences</h3> | |
| <h3>Email notification preferences</h3> |
There was a problem hiding this comment.
I think we are moving forward with Title Case, right? We can consider switching to Sentence case in the future. 🌽
| const ctx = useContext(UserContext); | ||
| const user = ctx.user; | ||
| const isTransactionalMail = !user?.additionalData?.emailNotificationSettings?.disallowTransactionalEmails; | ||
| const toggleTransactionalMail = async ()=>{ | ||
| if (user) { | ||
| user.additionalData = { |
There was a problem hiding this comment.
Maybe get user.additionalData inside the handlers?
Getting it outside of it means that you will overwrite any additionalData changes that happened between:
- This component was rendered
- A checkbox was ticked
Proof of concept: https://youtu.be/bYD8s3yivEw (my IDE change gets overwritten)
There was a problem hiding this comment.
The user is cached within the app. So in order to solve the (unlikely) case that someone changes settings in two browser tabs, we'd need to fetch the user from the server before altering it.
svenefftinge
force-pushed
the
se/notifications
branch
from
153169f
to
c66fc1f
Compare
Fixes #3398