-
Notifications
You must be signed in to change notification settings - Fork 213
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
feat(RP): Display subscription ToS/PP when client is Relay #18168
Conversation
isDesktopRelay = false, | ||
}: TermsPrivacyAgreementProps) => { | ||
return ( | ||
<div | ||
className={`text-grey-500 text-xs ${isDesktopRelay ? 'mt-8' : 'mt-5'}`} | ||
> | ||
{isPocketClient || isMonitorClient || isDesktopRelay ? ( | ||
{isPocketClient || isMonitorClient || isDesktopRelay || isRelayClient ? ( |
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.
Since we check for so many of these now, I think we might want a follow up issue to just take in clientId
here and let this component do the checks. The way we have it now does make storybook and tests easier for this component, though. (We'd just be moving some mocking from other components into mocking for this.)
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.
Since three of these conditions display terms for subscription services, we could have a isSubscriptionsService
function (mentioning as part of the possible follow-up, not required for this PR)
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.
Sure, though not all subscription services want this displayed and isDesktopRelay
is kind of a weird one. Maybe just shouldShowSubscriptionTOS
or something 🤷♀️
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.
Tested locally by adding the 123Done client id to the client matching/mixin, works as expected! 🎉
One requested change (though it doesn't really have any functional impact!), and one question:
- would you also like to add the relay client check to the reactified index page?
isDesktopRelay = false, | ||
}: TermsPrivacyAgreementProps) => { | ||
return ( | ||
<div | ||
className={`text-grey-500 text-xs ${isDesktopRelay ? 'mt-8' : 'mt-5'}`} | ||
> | ||
{isPocketClient || isMonitorClient || isDesktopRelay ? ( | ||
{isPocketClient || isMonitorClient || isDesktopRelay || isRelayClient ? ( |
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.
Since three of these conditions display terms for subscription services, we could have a isSubscriptionsService
function (mentioning as part of the possible follow-up, not required for this PR)
Because: * We should display these ToS/PP when users are signing up for Relay This commit: * Adds logic to account for this in content-server for email-first, and React for signin/signup closes FXA-10819
Because:
This commit:
closes FXA-10819