-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
New notifications experience #156
Conversation
Per post notifications and email digests are now controlled on a per group basis rather then globally New notification settings UI
apps/web/src/routes/UserSettings/NotificationSettingsTab/NotificationSettingsTab.js
Outdated
Show resolved
Hide resolved
return ( | ||
<MenuLink | ||
to={itemUrl} | ||
externalLink={item?.customView?.type === "externalLink" ? item.customView.externalLink : null} | ||
className="text-base text-foreground border-2 border-foreground/20 hover:border-foreground/100 hover:text-foreground rounded-md p-2 bg-background text-foreground mb-[.5rem] w-full transition-all scale-100 hover:scale-105 opacity-85 hover:opacity-100 flex align-items justify-between" | ||
externalLink={item?.customView?.type === 'externalLink' ? item.customView.externalLink : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I plan to go over all of this and double-check if things can be refractored from aarons changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was going to go over better use of MenuLink with him at some point too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs Mobile me
query update as well, as per note on schemes.graphql
. Rest of comments are trivial or just curiosities.
Per post notifications and email digests are now controlled on a per group basis rather then globally
New notification settings UI