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

bring in email preferences page from lbry.com #4409

Merged
merged 2 commits into from
Jun 22, 2020
Merged

Conversation

neb-b
Copy link

@neb-b neb-b commented Jun 19, 2020

The notifications PR was already pretty big so I decided to strip this out and make a PR for this separately.

@neb-b neb-b requested a review from kauffj June 19, 2020 20:15
@lbry-bot lbry-bot assigned kauffj and unassigned neb-b Jun 19, 2020
@neb-b
Copy link
Author

neb-b commented Jun 19, 2020

@kauffj This works when signed in, or if a verification_token param is found in the url.

lbry.tv/$/settings/notifications?verification_token={...}

/>
<>
<Card
title={__('Notifications')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could introduce tabs to settings instead of this? (not blocking feedback, can evolve)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. This definitely a step towards some tabbed/nested setting page.

{enabledEmails.map(({ email, isEnabled }) => (
<FormField
type="checkbox"
name={`active-email:${email}`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this safe for all potential emails?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It's just used for the input label for screen readers


{enabledEmails && enabledEmails.length > 0 && (
<Card
title={__('Receiving Addresses')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many people will have one email, probably worth making this conditional

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Will change.

@lbry-bot lbry-bot assigned neb-b and unassigned neb-b Jun 22, 2020
@neb-b neb-b merged commit a17610d into master Jun 22, 2020
@neb-b neb-b deleted the email-notifications-page branch June 22, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants