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

Fix: Limit donation amount input to 9 characters (#13872) #13877

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shahrukhahmed94
Copy link

@shahrukhahmed94 shahrukhahmed94 commented Jan 4, 2025

  • Added validation in MoneyFilter to restrict input to 9 characters.
  • Updated both filter and afterTextChanged methods to enforce this limit.
  • Ensured edge cases and formatting remain functional.
  • Resolves Limit for Donation Amount #13872.

First time contributor checklist

Contributor checklist

  • Device A, Android X.Y.Z
  • Device B, Android Z.Y
  • Virtual device W, Android Y.Y.Z
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

- Added validation in MoneyFilter to restrict input to 9 characters.
- Updated both filter and afterTextChanged methods to enforce this limit.
- Ensured edge cases and formatting remain functional.
- Resolves signalapp#13872.
@Sagar0-0
Copy link
Contributor

Sagar0-0 commented Jan 5, 2025

I am already committed to all the tickets I am raising. Please ask in the ticket comments before getting started with any fixes. This is also not to waste your time in redundant fixes and for other contributors like me who are working hard on each ticket.

@shahrukhahmed94
Copy link
Author

Hi Sagar, Thanks for pointing out this, I wasn't aware of that you are committed to all the issues that you raised, since this is my first contribution to this repo next time I'll be careful.

@Sagar0-0
Copy link
Contributor

Sagar0-0 commented Jan 5, 2025

Please make sure that either you have raised the issue or the issue is assigned to you before you start working on any. This is not a good practice as this may lead to multiple contributors working on the same issue. Please close this PR>

@shahrukhahmed94
Copy link
Author

Understood, I’ll close this PR now. Thank you for explaining the process, and I’ll ensure to check the issue assignments or comments before working on any issues in the future. Best of luck with resolving this issue!

@XSm2i
Copy link

XSm2i commented Jan 8, 2025

@Sagar0-0 It's not a good practice to assertively misinform a fellow user.

Please make sure that either you have raised the issue or the issue is assigned to you before you start working on any.

Nor ask them to close their PRs.

As it's fully baked and ready to be merged as is, I invite you instead to express what @shahrukhahmed94 could improve.


For small things like this we don't need a full issue / follow-up, you can just go ahead and submit a PR with some conversions.

@Sagar0-0
Copy link
Contributor

Sagar0-0 commented Jan 9, 2025

@XSm2i I am not misinforming anything here, I'm trying to help him to avoid making any beginner's mistakes.

The issue should be acknowledged by maintainers either it is an actual issue or meeds to be fixed. In this case we assume yes it is.

It's a good practice to ALWAYS first inform in the ticket before starting working on it(BETTER to be assigned) so that multiple contributors won't start working on it.

Also, for this issue is was never approved by maintainers that the solution for 9 CHARACTERS works for the solution, what I've learnt in open-source is we GENERALLY should not raise PRs for what we think works good as a solution.

@greyson-signal
Copy link
Contributor

Hey folks, just to clear things up:

I'm happy to look at any PR, regardless of whether there's an accompanying issue or not. Just be aware that I may not accept it for any number of reasons. But people are free to spend their time as they wish, propose any change they like, and non-Signal-employees should not tell other people what to do with their pull requests.

I appreciate anyone who takes time out of their day to report issues, and I'm very thankful for all of the pull requests that we've merged from volunteer contributors. Thanks!

@Sagar0-0
Copy link
Contributor

Sagar0-0 commented Jan 9, 2025

I completely agree, I shouldn't be the one to tell him to close the PR. Apologies to @shahrukhahmed94 for that.
Although all the advice I have shared is a general development cycle that I have seen being followed by many open-source projects, I shared the same, however, it's completely up to the project's specific guidelines what matters the most. I hope @shahrukhahmed94 learns the most out of this contribution. Thanks!

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.

Limit for Donation Amount
4 participants