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

Allow users to clear locally stored donation history #5196

Merged
merged 6 commits into from
Jan 15, 2025

Conversation

cesonha
Copy link
Contributor

@cesonha cesonha commented Dec 29, 2024

What does this do?

This will add a new preference in the SettingsFragment that enables the user to delete the local donation history. All this done respecting the business rules defined in the Phabricator ticket. The new preference only shows if the donation history is not empty and after the user deletes it the preference hides.

Why is this needed?

Please refer to the Phabricator's link Background section
Phabricator:
https://phabricator.wikimedia.org/T380662

Screenshot

image
image

@cesonha
Copy link
Contributor Author

cesonha commented Dec 29, 2024

Hi everyone, this is my first contribution and I have some doubts that I would be happy to get some clarification:

  • I saw that the iOS and Android Settings screen have a different UI and the preferences are organized in different sections, how can I know that the way I did is approved by the UX/UI team?
  • What should I do regarding the translation/localization, do I need to do it by myself?
  • What about unit tests, I understand that some classes might be easier to test than others, is there a discussion for that? I want to add tests to my PR but some guidance would be nice!
  • Do we need a summary for this preference?

@cesonha cesonha force-pushed the T380662 branch 3 times, most recently from 36061be to 90aa6bc Compare December 29, 2024 03:24
@cesonha cesonha changed the title [WIP] Feat: T380662 Allow users to clear locally stored donation history Dec 29, 2024
@cooltey
Copy link
Collaborator

cooltey commented Jan 3, 2025

Hi @cesonha, thanks for submitting the PR!

Hi everyone, this is my first contribution and I have some doubts that I would be happy to get some clarification:

* I saw that the iOS and Android Settings screen have a different UI and the preferences are organized in different sections, how can I know that the way I did is approved by the UX/UI team?

If you have claimed the ticket, you can tag HNordeenWMF or SChekfa-WMF by attaching the screenshot you have in the comment.

* What should I do regarding the translation/localization, do I need to do it by myself?

The way we add a translation, we will add the same <string name="..."> you have added to the default strings.xml to the qq/strings.xml, with a description of the string item.

* What about unit tests, I understand that some classes might be easier to test than others, is there a discussion for that? I want to add tests to my PR but some guidance would be nice!

I do not think this PR would need a test case.

* Do we need a summary for this preference?

You can put this comment in the Phabacator ticket and tag our PM ( the one who created the ticket).

@cooltey cooltey added the WIP Work in progress label Jan 3, 2025
@cesonha
Copy link
Contributor Author

cesonha commented Jan 4, 2025

Thanks @cooltey , just did as you've said and commented in phabricator tagging them!

@cesonha cesonha force-pushed the T380662 branch 3 times, most recently from 414e113 to e3aae6e Compare January 11, 2025 20:38
@cesonha
Copy link
Contributor Author

cesonha commented Jan 11, 2025

Hi @cooltey, it seems to be approved by the design and product team :) I've rebased with the main branch and added the string descriptions as you've said we should, should I just wait for code review/testing? :) TY

@cooltey cooltey removed the WIP Work in progress label Jan 13, 2025
@cesonha cesonha requested a review from cooltey January 15, 2025 00:21
Copy link
Collaborator

@cooltey cooltey left a comment

Choose a reason for hiding this comment

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

All looks good! Thanks for the updates.

@cooltey cooltey merged commit 2706625 into wikimedia:main Jan 15, 2025
1 check passed
@cesonha cesonha deleted the T380662 branch January 17, 2025 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants