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

Add terms to signin/settings pages #1971

Merged
merged 5 commits into from
Mar 23, 2021
Merged

Add terms to signin/settings pages #1971

merged 5 commits into from
Mar 23, 2021

Conversation

puneetlath
Copy link
Contributor

@puneetlath puneetlath commented Mar 22, 2021

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>

Details

1. Adds terms of service and privacy policy links to settings modal. 2. Adds terms of service and privacy policy links (only) on mobile apps/desktop 3. Adds terms of service, privacy policy, and licenses links on web/mobile web

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/154672

Tests

  1. Go to the sign in page on web & mobile web
  2. Make sure you see terms of service, privacy policy, and licenses links and that they work
  3. Go to the sign in page on ios/android/desktop
  4. Make sure you see only the terms of service and privacy policy links and that they work
  5. Sign in on each platform and tap your avatar to launch the settings modal
  6. Make sure you see the terms/privacy policy links at the bottom of the modal and that they work

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image
image

Mobile Web

image
image

Desktop

image
image

iOS

image
image

Android

image
image

@puneetlath puneetlath self-assigned this Mar 22, 2021
@puneetlath puneetlath requested a review from a team as a code owner March 22, 2021 04:29
@botify botify requested review from deetergp and removed request for a team March 22, 2021 04:29
@puneetlath puneetlath changed the title [WIP] Add terms to signin/settings pages Add terms to signin/settings pages Mar 22, 2021
@puneetlath
Copy link
Contributor Author

@deetergp this is ready for you!

cc @Expensify/design can you double-check that the spacing looks okay?

loginTermsText: {
color: themeColors.textSupporting,
fontSize: variables.fontSizeSmall,
lineHeight: 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try backing this down to 16 and seeing what it feels like?

We also need to include our font family here so that we serve this up in the correct font. Just add fontFamily: fontFamily.GTA, and we should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Here's what it looks like on web and android.

image

image

@megankelso

This comment has been minimized.

@@ -0,0 +1,3 @@
import TermsOnly from './TermsOnly';
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Why aren't we including licenses in desktop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only a banking requirement on web as explained here: https://github.com/Expensify/Expensify/issues/154672#issue-810980012

Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

Looks good on all platforms 🎉

@deetergp deetergp merged commit ddcebe8 into master Mar 23, 2021
@deetergp deetergp deleted the puneet-terms-licenses branch March 23, 2021 00:55
@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2021
@puneetlath
Copy link
Contributor Author

@megankelso it looks like that copy was written here: https://github.com/Expensify/Expensify/issues/155153

So maybe reopen that issue to see what people think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants