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

Link Verification UI #9606

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Link Verification UI #9606

wants to merge 4 commits into from

Conversation

toluo-stripe
Copy link
Contributor

@toluo-stripe toluo-stripe commented Nov 12, 2024

Summary

Native UI for verifying link account

Motivation

JIRA

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

382094464-20185f3d-1d48-485c-ba77-048123fde4bd.mov

Changelog

Copy link
Contributor

github-actions bot commented Nov 12, 2024

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 301.8 KiB │ 301.8 KiB │  0 B │ 455.5 KiB │ 455.5 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.2 KiB │  90.2 KiB │ -4 B │ 170.3 KiB │ 170.3 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ -4 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19966 │ 19966 │ 0 (+0 -0) 
   types │  6188 │  6188 │ 0 (+0 -0) 
 classes │  4979 │  4979 │ 0 (+0 -0) 
 methods │ 29759 │ 29759 │ 0 (+0 -0) 
  fields │ 17526 │ 17526 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3622 │ 3622 │  0
APK
   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
    268 B │ -3 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
 28.5 KiB │ -2 B │  62.9 KiB │  0 B │ ∆ META-INF/CERT.SF                        
 25.3 KiB │ +1 B │  62.8 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
   54 KiB │ -4 B │ 125.8 KiB │  0 B │ (total)

@toluo-stripe toluo-stripe changed the title verification ui Link Verification UI Nov 13, 2024
@toluo-stripe toluo-stripe marked this pull request as ready for review November 13, 2024 17:55
@toluo-stripe toluo-stripe requested review from a team as code owners November 13, 2024 17:55
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "Enter your vertification code" text looks way bigger on these screenshots than on the screen recording you uploaded. Do you know why that is?

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 because it's not wrapped with the correct theme. Just fixed it

Comment on lines 21 to 22
listOf(SystemAppearance.DarkTheme),
listOf(FontSize.DefaultFont)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend only including config options if you're going to list multiple options here -- otherwise, you can just leave them out and it'll default to our default settings

style = MaterialTheme.typography.body1,
color = MaterialTheme.colors.onSecondary
)
DefaultStripeTheme {
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of curiosity - why does this theme only apply to the otp element and not to the whole screen? Or why are we not using the link theme for this part of the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it because the old UI wrapped the component in this theme, but it looks like removing it doesn't change anything. I just took it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some UI tests for this screen? Generally we use compose rule to test behavior of our UIs

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

Successfully merging this pull request may close these issues.

2 participants