Skip to content

Make Terms of Service and Privacy Policy accessible#5173

Closed
elemdos wants to merge 12 commits into
Expensify:mainfrom
elemdos:issue-5082
Closed

Make Terms of Service and Privacy Policy accessible#5173
elemdos wants to merge 12 commits into
Expensify:mainfrom
elemdos:issue-5082

Conversation

@elemdos

@elemdos elemdos commented Sep 9, 2021

Copy link
Copy Markdown
Contributor

Details

See fixed issue

Fixed Issues

$ #5082

Tests

  1. Open Settings screen
  2. Click 'About'
  3. Tab through About options
  4. Verify footer links (Terms of Service, Privacy Policy) focus when tabbed

QA Steps

  1. Open Settings screen
  2. Click 'About'
  3. Tab through About options
  4. Verify footer links (Terms of Service, Privacy Policy) focus when tabbed

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Web.mov

Mobile Web

mobileweblinkopens.mov

Desktop

desktoplinks.mov

iOS

ios.mov

Android

androidscreen.mov

@elemdos elemdos requested a review from a team as a code owner September 9, 2021 20:01
@MelvinBot MelvinBot requested review from johnmlee101 and removed request for a team September 9, 2021 20:01
@github-actions

github-actions Bot commented Sep 9, 2021

Copy link
Copy Markdown
Contributor

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@elemdos

elemdos commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@johnmlee101 johnmlee101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you quickly demonstrate that on any mobile device that there are no regressions in terms of the links working?

@elemdos

elemdos commented Sep 12, 2021

Copy link
Copy Markdown
Contributor Author

@johnmlee101 sorry for the delay, still trying to get ios/android set up locally

@elemdos

elemdos commented Sep 12, 2021

Copy link
Copy Markdown
Contributor Author

There was a styling issue on iOS

image

So I updated the TextLinkss to use styles.textMicroSupporting instead of chatItemMessageHeaderTimestamp (which was setting a static height on the link). Looks great now. This also solves the clipping (i.e. pipes) problem brought up in the issue. I've updated the demos accordingly.

Will have the Android & Mobile Web demos up tomorrow.

@johnmlee101

Copy link
Copy Markdown
Contributor

Amazing! It all comes together 😄
Looking forward to the rest of the updated demos.

@elemdos

elemdos commented Sep 15, 2021

Copy link
Copy Markdown
Contributor Author

@johnmlee101 sorry for the delay! Getting android set up was a doozy. Last demo added.

On that note, there's a visual regression now with the links on Android. Guessing it needs its own issue?

@kadiealexander

Copy link
Copy Markdown
Contributor

Hi @MateoMorris, I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting.

@johnmlee101

Copy link
Copy Markdown
Contributor

Hello! This is no longer on hold. Anything left to do here other than merge with main?

@johnmlee101

Copy link
Copy Markdown
Contributor

Friendly bump!

@elemdos

elemdos commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

@johnmlee101 sorry, been having some issues getting Android running locally. Just need that and this should be good to merge.

@elemdos

elemdos commented Dec 15, 2021

Copy link
Copy Markdown
Contributor Author

@johnmlee101 okay ready to merge!

johnmlee101
johnmlee101 previously approved these changes Dec 15, 2021
@johnmlee101

Copy link
Copy Markdown
Contributor

Oh no! All commits need to be signed and your last commit wasn't. You might need to rebase locally and either force push this branch if possible or create a new PR and we can quickly move on that.

@elemdos

elemdos commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

@johnmlee101 not really sure I understand that, but I merged master back into this branch and it looks like that's Verified because it was done from Github. Is that what you're looking for?

@johnmlee101 johnmlee101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me see

@johnmlee101

Copy link
Copy Markdown
Contributor

image

@johnmlee101

Copy link
Copy Markdown
Contributor

@johnmlee101 johnmlee101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me know once that is done!

@elemdos

elemdos commented Dec 23, 2021

Copy link
Copy Markdown
Contributor Author

Okay @johnmlee101 figured out code signing. How should I proceed?

@johnmlee101

Copy link
Copy Markdown
Contributor

The best way to do this is to try a git rebase -f main (I think this works with forks?) and then force push to your branch with git push -f origin mateomorris:issue-5082

If the second part doesn't work, create a new PR, reference and close this one, and then we'll move it to there!

@elemdos

elemdos commented Jan 5, 2022

Copy link
Copy Markdown
Contributor Author

Back at this after the holidays - should have it done today if everything goes smoothly 🤞

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.

4 participants