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

Update PaymentSheet bottom paddings & error padding #9198

Conversation

samer-stripe
Copy link
Collaborator

@samer-stripe samer-stripe commented Sep 6, 2024

Summary

Update PaymentSheet bottom paddings & error padding

Motivation

Design decided to put the error message above the bottom content padding but with some extra padding between it and the rest of the card content. Also more accurately updated the PaymentSheet padding so that horizontal and vertical mode are matched.

Mock

Testing

  • Added tests
  • Modified tests
  • Manually verified

@@ -223,7 +223,7 @@ internal sealed interface PaymentSheetScreen {

@Composable
override fun Content(modifier: Modifier) {
AddPaymentMethod(interactor = interactor, modifier)
AddPaymentMethod(interactor = interactor)
Copy link
Collaborator Author

@samer-stripe samer-stripe Sep 6, 2024

Choose a reason for hiding this comment

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

We weren't passing the modifier for the VerticalModeFormUI as well. Looks like because the form applies its own padding. Should we move the default modifier that applies 8.dp as bottom padding into the individual screens?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the individual screens should be responsible for their own padding. In this case, we would need to add 8.dp to all the screens individually? So it just makes more sense to apply that padding globally.

Rather than doing this, I think we should remove the padding from the forms, if we need to

Copy link
Contributor

github-actions bot commented Sep 6, 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 │   1.9 MiB │   1.9 MiB │  0 B │     4 MiB │     4 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.5 KiB │ 301.5 KiB │  0 B │   455 KiB │   455 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   6.8 KiB │   6.8 KiB │  0 B │   6.6 KiB │   6.6 KiB │  0 B 
    other │  85.5 KiB │  85.5 KiB │ -1 B │ 158.7 KiB │ 158.7 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.5 MiB │   9.5 MiB │ -1 B │  21.4 MiB │  21.4 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 20269 │ 20269 │ 0 (+0 -0) 
   types │  6106 │  6106 │ 0 (+0 -0) 
 classes │  4913 │  4913 │ 0 (+0 -0) 
 methods │ 29561 │ 29561 │ 0 (+0 -0) 
  fields │ 17371 │ 17371 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3608 │ 3608 │  0
APK
   compressed    │  uncompressed   │                                           
──────────┬──────┼──────────┬──────┤                                           
 size     │ diff │ size     │ diff │ path                                      
──────────┼──────┼──────────┼──────┼───────────────────────────────────────────
 25.1 KiB │ -2 B │ 62.5 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
    272 B │ +1 B │    120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
──────────┼──────┼──────────┼──────┼───────────────────────────────────────────
 25.4 KiB │ -1 B │ 62.6 KiB │  0 B │ (total)

@samer-stripe samer-stripe force-pushed the samer/update-payment-sheet-screen-padding-with-errors branch 2 times, most recently from 478a44e to 6a11c5e Compare September 9, 2024 16:04
@samer-stripe samer-stripe force-pushed the samer/update-payment-sheet-screen-padding-with-errors branch from 6a11c5e to f4413bf Compare September 9, 2024 16:19
@samer-stripe samer-stripe marked this pull request as ready for review September 9, 2024 16:20
@samer-stripe samer-stripe requested review from a team as code owners September 9, 2024 16:20
@samer-stripe samer-stripe changed the title Update PaymentSheet bottom paddings with error Update PaymentSheet bottom paddings & error padding Sep 9, 2024
@@ -223,7 +223,7 @@ internal sealed interface PaymentSheetScreen {

@Composable
override fun Content(modifier: Modifier) {
AddPaymentMethod(interactor = interactor, modifier)
AddPaymentMethod(interactor = interactor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the individual screens should be responsible for their own padding. In this case, we would need to add 8.dp to all the screens individually? So it just makes more sense to apply that padding globally.

Rather than doing this, I think we should remove the padding from the forms, if we need to

Comment on lines 39 to +40
internal val verticalModeBottomContentPadding = 20.dp
internal val horizontalModeBottomContentPadding = 8.dp
internal val horizontalModeBottomContentPadding = 20.dp
Copy link
Collaborator

Choose a reason for hiding this comment

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

if these have the same value, do we need to set this per-screen at all? Or can we just use 20.dp as bottom content padding in PaymentSheetScreen?

@samer-stripe samer-stripe marked this pull request as draft September 19, 2024 06:21
@samer-stripe
Copy link
Collaborator Author

Dropping this for now. Will be re-visited during embedded.

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