feat: combine payment buttons and Card Fields into one Demo fragment - #1702
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The navigation and Demo consolidation look consistent, with only a minor UI-string localization nit identified.
Pull request overview
This PR streamlines the Demo app’s Compose entry points by consolidating the standalone “Payment Buttons Compose” and “Card Fields Compose” screens into a single Compose-based UI Components fragment, reducing fragmentation in the Demo navigation and surface area.
Changes:
- Replace two Compose Demo entry buttons with a single “UI Components Compose” entry.
- Update the Demo navigation graph to remove the old Compose fragments and route to the new combined fragment.
- Add
ComposeUIComponentsFragmentthat hosts PayPal/Venmo Compose buttons alongside Card Fields in one screen.
File summaries
| File | Description |
|---|---|
| Demo/src/main/res/values/strings.xml | Replaces two Compose button labels with a single “UI Components Compose” string. |
| Demo/src/main/res/navigation/nav_graph.xml | Removes old Compose fragment destinations and adds a new combined Compose destination + actions. |
| Demo/src/main/java/com/braintreepayments/demo/MainFragment.kt | Updates the Compose entry button and navigation to the new combined fragment. |
| Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt | New combined Compose fragment integrating PayPal/Venmo buttons and Card Fields submission flow. |
| Demo/src/main/java/com/braintreepayments/demo/ComposeCardFieldsFragment.kt | Deleted; functionality now covered by the combined Compose fragment. |
| Demo/src/main/java/com/braintreepayments/demo/ComposeButtonsFragment.kt | Deleted; functionality now covered by the combined Compose fragment. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
noguier
changed the title
saralvasquez
left a comment
saralvasquez
left a comment
There was a problem hiding this comment.
This looks great! The only thing I noticed during testing that might be worth coming back to is that the color selected for the buttons doesn't survive recomposition so it defaults back to blue whenever the screen is rotated. But I don't know if that's worth digging into now since this is just about bringing the UI components together into one screen
|
Op I missed the lint failure. Looks like a couple things detekt is angry about |
c0be0c8 to
46b0321
Compare
84309e2 to
42db49f
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The new Compose screen introduces a hardcoded UI string that should use existing string resources for localization/consistency.
Review details
Suppressed comments (1)
Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt:184
- The Card Fields submit button label is hardcoded ("Pay"), which bypasses existing string resources/localization. Since
@string/card_fields_payalready exists and is used by the XML UI components screen, prefer usingstringResource(...)here for consistency and translatability.
Text("Pay")
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
CHANGELOG.md currently breaks release automation (header casing) and contains an Unreleased note that does not reflect the actual changes in this PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There are correctness/clarity issues to address in the new Compose fragment lifecycle setup and the added CHANGELOG entry does not accurately describe the PR’s changes.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt:73
setViewCompositionStrategy(...)should be set before the firstsetContent { ... }call; setting it afterwards may not apply to the already-created composition, and you risk using the default disposal strategy for this Fragment's ComposeView.
This issue also appears on line 187 of the same file.
Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt:254
PayPalRequestFactory.createPayPalCheckoutRequest(...)is called with many positional arguments here, which is hard to review and brittle if the factory signature changes. Other Demo code documents these arguments inline; doing the same here will make intent clearer.
Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt:189
- After setting the composition strategy before
setContent, this later call becomes redundant and should be removed to avoid confusion about which strategy is in effect.
}
setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed)
}
CHANGELOG.md:6
- This unreleased CHANGELOG entry describes adding Compose
CardFieldssupport, but this PR only reorganizes the Demo Compose UI Components screen (the Compose CardFields demo already existed). Please update the entry to describe the Demo change so the release notes remain accurate.
* UIComponents
* Add Compose `CardFields` support to generate a premade credit card form for submitting credit card tokenize requests
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Great catch, I confirmed that this behavior was present in the Compose PaymentButtons and is not caused by this change, since it feels a little out of scope for this PR I will create a separate ticket to address that! Thank you! |
buzzamus
left a comment
buzzamus
left a comment
There was a problem hiding this comment.
Looks good! I had one tiny nit and another take it or leave it, but nothing blocking here.
| "https://mobile-sdk-demo-site-838cead5d3ab.herokuapp.com/braintree-payments" | ||
| private const val DEEP_LINK_FALLBACK_SCHEME = "com.braintreepayments.demo.braintree" | ||
|
|
||
| @Suppress("LongMethod") |
There was a problem hiding this comment.
Should this just be placed on the offending method, or are there several causing issues in this file?
There was a problem hiding this comment.
Great catch thank you!
| savedInstanceState: Bundle? | ||
| ): View { | ||
| super.onCreateView(inflater, container, savedInstanceState) | ||
| val payPalRequest = paypalRequest(requireContext()) |
There was a problem hiding this comment.
nit: variable name is payPal camelcase while paypalRequest has both P's lower
| } | ||
| } | ||
|
|
||
| private fun handleNonce(nonce: PaymentMethodNonce) { |
There was a problem hiding this comment.
nice! Love the consolidation of the separate result handling 😎
There was a problem hiding this comment.
🔵 Needs a closer look
Address the two moderate findings before approval.
Review details
Suppressed comments (2)
CHANGELOG.md:6
- This release-note entry claims that this PR adds Compose CardFields support, but the deleted
ComposeCardFieldsFragmentalready used the existing Compose CardFields API; this change only moves that Demo UI into the combined fragment. Please describe the Demo reorganization instead so the changelog does not attribute an SDK feature addition to this patch.
* Add Compose `CardFields` support to generate a premade credit card form for submitting credit card tokenize requests
Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt:94
- The Compose payment-button implementations render their inner row at a fixed 300dp width (
UIComponents/.../PayPalButtonView.kt:47-49,82-87and the equivalent Venmo view). Each weighted column here is only about half the phone width, so the buttons overflow and overlap on normal portrait devices. Make this layout responsive (for example, stack the columns when they cannot fit) or update the button composables to honor the available width.
Row(modifier = Modifier.fillMaxWidth()) {
Column(modifier = Modifier.weight(1f)) {
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the changelog wording, avoid the class-level API test suppression, and fix the suppression message grammar.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
CHANGELOG.md:6
- This release-note entry claims that this change adds Compose
CardFieldssupport, but the base already containsComposeCardFieldsFragment; this PR only moves that existing Demo implementation into the combined fragment. As written, the changelog falsely suggests a new SDK capability, so please describe the Demo reorganization or omit the entry if Demo-only changes are not released.
* UIComponents
* Add Compose `CardFields` support to generate a premade credit card form for submitting credit card tokenize requests
LocalPayment/src/androidTest/java/com/braintreepayments/api/localpayment/LocalPaymentApiTest.kt:15
- The new suppression message has a grammatical error: “This test are” should be “These tests are” (or “This test is”).
@Ignore("This test are failing intermittently, LocalPayment team is investigating")
LocalPayment/src/androidTest/java/com/braintreepayments/api/localpayment/LocalPaymentClientTest.kt:25
- Applying
@Ignoreto the whole class removes everyLocalPaymentClientTestcase from the connected test suite, including the validation and browser-switch result coverage. This Demo-only refactor should not silently reduce unrelated LocalPayment coverage; remove the class-level ignore, or isolate only the known flaky methods and track the root cause.
@Ignore("This test are failing intermittently, LocalPayment team is investigating")
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
| import org.junit.Ignore | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
|
|
||
| @Ignore("This test are failing intermittently, LocalPayment team is investigating") |
| import org.junit.runner.RunWith | ||
| import java.util.concurrent.CountDownLatch | ||
|
|
||
| @Ignore("This test are failing intermittently, LocalPayment team is investigating") |
There was a problem hiding this comment.
🔵 Needs a closer look
Correct the changelog and button layout, and remove or narrowly scope the LocalPayment test ignores.
Review details
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
Demo/src/main/java/com/braintreepayments/demo/ComposeUIComponentsFragment.kt:97
- Each payment button renders a fixed 300dp-wide row, but this
Rowgives each weighted column only about half the screen width after padding and spacing. On typical phones the PayPal and Venmo buttons will overflow or be clipped; keep these fixed-width buttons in full-width containers or use a responsive layout instead of placing them side by side.
LocalPayment/src/androidTest/java/com/braintreepayments/api/localpayment/LocalPaymentClientTest.kt:25 - This class-level ignore disables every LocalPayment client instrumentation test, so CI can no longer detect regressions in this suite. The combined UI-components Demo change is unrelated to these tests; please remove this ignore and address or narrowly scope the intermittent test failures with a tracking issue instead of suppressing the whole class.
CHANGELOG.md:6
- The PR reuses the existing Compose
CardFieldsAPI in the Demo app; it does not add SDK support. This entry therefore misstates the change and duplicates the existingCardFieldsrelease note atCHANGELOG.md:79-80. Replace it with a Demo-specific description of the combined fragment, or omit the entry if Demo-only changes are not released.
* UIComponents
* Add Compose `CardFields` support to generate a premade credit card form for submitting credit card tokenize requests
LocalPayment/src/androidTest/java/com/braintreepayments/api/localpayment/LocalPaymentApiTest.kt:15
- This class-level ignore disables every LocalPayment API instrumentation test, so CI can no longer detect regressions in this suite. The combined UI-components Demo change is unrelated to these tests; please remove this ignore and address or narrowly scope the intermittent test failures with a tracking issue instead of suppressing the whole class.
@Ignore("This test are failing intermittently, LocalPayment team is investigating")
LocalPayment/src/androidTest/java/com/braintreepayments/api/localpayment/LocalPaymentApiTest.kt:15
- The ignore rationale has a grammar error and is missing the article before the team name. Use “These tests are failing intermittently; the LocalPayment team is investigating.”
@Ignore("This test are failing intermittently, LocalPayment team is investigating")
LocalPayment/src/androidTest/java/com/braintreepayments/api/localpayment/LocalPaymentClientTest.kt:25
- The ignore rationale has a grammar error and is missing the article before the team name. Use “These tests are failing intermittently; the LocalPayment team is investigating.”
@Ignore("This test are failing intermittently, LocalPayment team is investigating")
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
* feat: add compose card number field (#1677) * feat: add compose card number functionality and demo * pr-feedback: fix spacing and round card corners * pr-feedback: rename base text input class * pr-feedback: dimens strings, error icon, content description, composition strategy * fix: card number description * fix: rename state class to controller * feat: add card fields compose expiration field (#1678) * feat: add compose card fields expiration date field * fix: lint issues * pr-feedback: consolidate test classes * fix: add contentDescription * fix: lint issue * fix: rename controller * fix: rename state in unit tests * feat: add compose cvv field (#1681) * feat: add cvv field * test: add unit tests * fix: lint issues * fix: resolve length truncation bug * fix: cvv popup spacing issue * fix: allow paste for cvv * fix: narrow screen expiration overflow * pr-feedback: dimens values * fix: add contentDescription * fix: complexity lint issue * fix: re-add value removed in merge conflict * pr-feedback: fix unit tests, add visual remember, resolve UI bug * feat: add compose pay button and processing (#1683) * feat: add compose pay button and processing * pr-feedback: update unit tests, add demo comment, fix process death bug, and remove initialize * feat: add compose card field analytics (#1685) * Compose autoadvance (#1687) * initial commit of the autoadvance * feat: add compose card fields expiration date field * fix: lint issues * pr-feedback: consolidate test classes * fix: fix lint * fix: fix KDoc wording * fix: address PR comment to abstract duplicate code --------- Co-authored-by: Sara Vasquez <saravasquez@paypal.com> * Compose CardFields Instrumentations Tests (#1692) * feat: add instrumentation tests to cover CardFields Compose module * fix: fix lint * fix: fix failing ci tests * fix: address PR comment to add more tests * feat: add compose card fields README entry (#1691) * feat: add readme entry for compose card fields * fix: tweak language * fix: updated code block to match controller * docs: add missing kdocs * fix: remove unnecessary param comments * feat: combine payment buttons and Card Fields into one Demo fragment (#1702) * feat: combine payment buttons and Card Fields into one Demo fragment to match XML * fix: address PR Comment and add `CHANGELOG` entry * fix: address PR Comment * fix: address PR Comments * fix: ignore failing test to ensure all other tests pass * fix: fix failing test in UI components Compose * pr-feedback: add cvv popup anchor and fix dp to sp conversion bug * fix: PR suggestions and a patch to resize text in PaymentButtons style buttons to fit smaller devices * fix: revert back to using KeyboardType.Number to allow pasting an input --------- Co-authored-by: Sara Vasquez <98496950+saralvasquez@users.noreply.github.com> Co-authored-by: Sara Vasquez <saravasquez@paypal.com>
Summary of changes
CHANGELOGentryDemo Video
Screen.Recording.2026-09-09.at.5.19.16.PM.mov
AI Usage
Which AI Agent Was Used?
How was AI used?
Claude was used as a first reviewer on the combined fragments work
Estimated AI Code Contribution
Checklist
Authors