-
Notifications
You must be signed in to change notification settings - Fork 35
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 can't send email warning for users that can not receiv… #176
Conversation
foysalit
commented
Sep 4, 2024
…e-email-based-on-pds
Your Render PR Server URL is https://ozone-staging-pr-176.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-crcvkrhu0jms73en51og. |
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.
UX LGTM
} | ||
|
||
return DOMAINS_ALLOWING_EMAIL_COMMUNICATION.some((domain) => { | ||
return pdsEndpoint.endsWith(domain) |
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.
We probably want the domain to end with .bsky.network
rather than bsky.network
.
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.
makes sense. but since this is env var, we can just configure it that way in the var, right? maybe the name needs an adjustment so that it's not DOMAINS_ALLOWING_EMAIL_COMMUNICATION and instead DOMAINS_SUFFIXES_ALLOWING_EMAIL_COMMUNICATION?
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.
@devinivy do lmk if you have thoughts on this, I'll merge for now and happy to address any different outcome from this as a fast follow.
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.
Yup we can just configure it that way 👍
lib/constants.ts
Outdated
export const DOMAINS_ALLOWING_EMAIL_COMMUNICATION = ( | ||
process.env.NEXT_PUBLIC_DOMAINS_ALLOWING_EMAIL_COMMUNICATION || 'bsky.network' |
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.
Perhaps we should default to empty, and configure this specially on our deployments— otherwise this will be the configuration in the distro.
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.
totally!
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.
Left a couple notes, but looking good 👍