Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #18268, #18269 - [Saved cards] Display a list of Saved cards fetched from the credit card storage #18808

Merged
merged 1 commit into from
Apr 8, 2021
Merged

For #18268, #18269 - [Saved cards] Display a list of Saved cards fetched from the credit card storage #18808

merged 1 commit into from
Apr 8, 2021

Conversation

gabrielluong
Copy link
Member

@gabrielluong gabrielluong commented Apr 5, 2021 •
edited
Loading

Fixes #18268, #18269

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@gabrielluong gabrielluong force-pushed the 18268-1 branch 5 times, most recently from d7b8ded to 3010c59 Compare April 6, 2021 15:54
@gabrielluong gabrielluong marked this pull request as ready for review April 6, 2021 15:54
@gabrielluong gabrielluong requested review from a team as code owners April 6, 2021 15:54
@gabrielluong gabrielluong added the needs:review PRs that need to be reviewed label Apr 6, 2021
@gabrielluong gabrielluong changed the title For #18268 - [Saved cards] Add a Saved cards screen for credit cards For #18268, #18269 - [Saved cards] Display a list of Saved cards fetched from the credit card storage Apr 7, 2021
@gabrielluong gabrielluong mentioned this pull request Apr 7, 2021
Closed
@Mugurell Mugurell added the pr:needs-ac-bump PR that needs a AC bump label Apr 7, 2021
Mugurell
Mugurell reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

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

This looks awesome!

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/settings/creditcards/view/CreditCardItemViewHolder.kt Outdated Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/settings/creditcards/view/CreditCardsManagementView.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/component_credit_cards.xml
Comment on lines +17 to +18
android:layout_height="8dp"
android:translationY="-3dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this mean it is has an effective height of 5 dp since it's already at the top of it's parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have any insight to this. This seems to be what was the most common styling for all the ProgressBar I have seen in our code.

app/src/test/java/org/mozilla/fenix/settings/creditcards/CreditCardItemViewHolderTest.kt Outdated Show resolved Hide resolved
@Mugurell
Copy link
Contributor

Mugurell commented Apr 7, 2021

This looks really nice but I saw that the current UX doesn't exactly maps the mockup posted in the ticket

expected actual
image image

 
Also saw that although the CreditCard instance is passed to the CreditCardEditorFragment that instance is not used to populate the card fields to actually "edit" an existing card.
Starting with the empty fields and pressing save at any time will create and store a new card, even one with all fields left empty.

Are there follow-up changes / tickets for this?

@Mugurell Mugurell removed the needs:review PRs that need to be reviewed label Apr 7, 2021
@gabrielluong
Copy link
Member Author

gabrielluong commented Apr 7, 2021 •
edited
Loading

Yes, we still need more work to get the icons and credit card validations in before we can determine what credit card issuer is for a given credit card that is done in mozilla-mobile/android-components#9813. Icons are to be added in #18271. Editing a credit card is done in #18272 and #18274. They are both waiting for your review immediately after this one.

I can remove the CreditCard argument for now to unblock the test breakage because it's not parcelable right now and waiting for mozilla-mobile/android-components#10027.

@gabrielluong gabrielluong force-pushed the 18268-1 branch 2 times, most recently from 4b8fa1e to 98ae631 Compare April 7, 2021 23:40
@gabrielluong gabrielluong added needs:review PRs that need to be reviewed and removed pr:needs-ac-bump PR that needs a AC bump labels Apr 7, 2021
@gabrielluong
For mozilla-mobile#18268, mozilla-mobile#18269 - [Saved cards] Displa…
30456bd 
…y a list of Saved cards fetched from the credit card storage
Mugurell
Mugurell approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

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

Thank you!
Looks like a nice skeleton that can be built on.

@gabrielluong gabrielluong merged commit 37b6079 into mozilla-mobile:master Apr 8, 2021
@gabrielluong gabrielluong deleted the 18268-1 branch April 8, 2021 16:24
@gabrielluong gabrielluong mentioned this pull request Apr 26, 2021
Merged
3 tasks
@gabrielluong gabrielluong mentioned this pull request May 18, 2021
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mugurell Mugurell Mugurell approved these changes

Assignees

@Mugurell Mugurell

Labels
needs:review PRs that need to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Saved cards] Add a Saved cards screen for credit cards
2 participants
@gabrielluong @Mugurell