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/payment request fixes #16936

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Fix/payment request fixes #16936

merged 3 commits into from
Dec 12, 2024

Conversation

Cuteivist
Copy link
Contributor

What does the PR do

  • Fixed wrong formatting of amount balance
  • Fixed rendering issue with Payment request preview mini card. I couldn't find the root cause, this code was only working solution I could find using the Layouts
  • Added symbol name to amount input in Payment Request Modal

Affected areas

Chat / Payment Request modal

Architecture compliance

Screenshot of functionality (including design for comparison)

image image

Impact on end user

No glitches and wrong formatting for payment requests

How to test

  • How should one proceed with testing this PR.
    Add payment request to status input and send it
  • What kind of user flows should be checked?
    Payment request

Risk

Described potential risks and worst case scenarios.

Tick one:

  • Low risk: 2 devs MUST perform testing as specified above and attach their results as comments to this PR before merging.
  • High risk: QA team MUST perform additional testing in the specified affected areas before merging.

@status-im-auto
Copy link
Member

status-im-auto commented Dec 11, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ fab9c94 #1 2024-12-11 10:03:44 ~8 min tests/nim 📄log
✔️ fab9c94 #1 2024-12-11 10:07:35 ~11 min macos/aarch64 🍎dmg
✔️ fab9c94 #1 2024-12-11 10:08:42 ~12 min tests/ui 📄log
✔️ fab9c94 #1 2024-12-11 10:12:53 ~17 min linux/x86_64 📦tgz
✔️ fab9c94 #1 2024-12-11 10:13:04 ~17 min macos/x86_64 🍎dmg
✔️ fab9c94 #1 2024-12-11 10:13:30 ~17 min linux-nix/x86_64 📦tgz
✔️ fab9c94 #1 2024-12-11 10:24:15 ~28 min windows/x86_64 💿exe

asset.name: Constants.tokenIcon(root.symbol)
}
}
contentItem: GridLayout {
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite get why you need the GridLayout here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow I got text to be rendered. Otherwise it won't render the text.

@anastasiyaig
Copy link
Contributor

it fixes the linked issues

@anastasiyaig anastasiyaig self-requested a review December 11, 2024 13:24
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I'll wait for a QML expert to give the thumbs up

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Agreed with Emil to attempt a better fix for master; this is good for the release

@alaibe alaibe merged commit 934d957 into release/2.32.x Dec 12, 2024
9 checks passed
@alaibe alaibe deleted the fix/payment-request-fixes branch December 12, 2024 13:19
@jrainville
Copy link
Member

@Cuteivist can you cherry-pick to master. Thanks

@anastasiyaig
Copy link
Contributor

@jrainville Emil is preparing proper fix for master, it wont be a cherry pick but new implementation

@jrainville
Copy link
Member

@jrainville Emil is preparing proper fix for master, it wont be a cherry pick but new implementation

Ah right. Perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants