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.

Sign up for GitHub

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

Jump to bottom

[ABW-2732] Fix transaction preview screen scroll #779

Merged
merged 8 commits into from
Feb 1, 2024
Merged

[ABW-2732] Fix transaction preview screen scroll #779

merged 8 commits into from
Feb 1, 2024

Conversation

giannis-rdx
Copy link
Contributor

@giannis-rdx giannis-rdx commented Jan 31, 2024 •
edited
Loading

Description

Edit: new solution by @micbakos-rdx was introduced here

This PR fixes:

  • the scrolling in transaction preview screen, by keeping the preview header in its expanded version when there is not enough space to scroll
  • all the paddings for the transaction preview header (top) and network fee content + slider (bottom) based on Zeplin
  • receipt edge now is always presented in the preview header and follows the scroll

If you guys have a more elegant/fancy solution, please suggest!

How to test

  1. Test it in both stokenet and mainnet by sending different transactions from dApps to the wallet. Be sure that when there is not enough space to scroll the preview header remains fixed in its expanded version.
  2. If you are a good guy then change the screen size of your device and test it again.

Video

Transaction.preview.scroll.Samsung.mp4
transaction.preview.scroll.xiaomi.mp4

PR submission checklist

  • I have tested with two different devices, on different networks, by hardcoded enabling/disabling the elements of the preview header (e.g. without subtitle) and with different screen/font sizes from the device settings. Because this is how you properly test this particular bug.

@giannis-rdx giannis-rdx self-assigned this Jan 31, 2024
giannis-rdx
giannis-rdx commented Jan 31, 2024
View reviewed changes
.../com/babylon/wallet/android/presentation/transaction/composables/TransactionPreviewHeader.kt Outdated
Comment on lines 70 to 72
// if max value of the scroll is less than the height of the preview header then don't use the animation
// the + 100 is for these cases where the warnings at the bottom of the receipt are visible (e.g. congested network)
if (scrollState.maxValue <= size.height + 100) {
Copy link
Contributor Author

@giannis-rdx giannis-rdx Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I don't add the +100 it flickers again. And I am still wondering why this happens. 🤷🏽‍♂️ It shouldn't with this check.
Screenshot 2024-01-31 at 1 10 53 PM

raf-rdx
raf-rdx reviewed Jan 31, 2024
View reviewed changes
.../com/babylon/wallet/android/presentation/transaction/composables/TransactionPreviewHeader.kt Outdated
R.raw.transaction_review_top_bar_scene
).readBytes().decodeToString()
.fillMaxWidth()
.onGloballyPositioned { layoutCoordinates ->
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

raf-rdx
raf-rdx approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@raf-rdx raf-rdx left a comment

Choose a reason for hiding this comment

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

Tested with multiple transactions and no flickering anymore 🎉

@micbakos-rdx
Copy link
Contributor

Unfortunately I could make it again flicker, this is easier to reproduce when the bottom bar is a bit bigger due to some warnings shown. Here is the commit 89837f9

The way it works is by copying the implementation of the LargeTopAppBar but by editing some hardcoded values that didn't allow us to achieve the same result with designs.

Internal transfer
External transfer with icon
Internal transfer with little scroll

jakub-rdx
jakub-rdx approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@jakub-rdx jakub-rdx left a comment

Choose a reason for hiding this comment

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

we should probably remove motion scene xml for previous solution if it is not used anymore

@micbakos-rdx
Copy link
Contributor

we should probably remove motion scene xml for previous solution if it is not used anymore

You are right forgot about that

@giannis-rdx giannis-rdx force-pushed the bugfix/ABW-2732-fix-transaction-preview-scroll branch from 5fc0a3c to fb6e3bb Compare February 1, 2024 08:59
@giannis-rdx giannis-rdx force-pushed the bugfix/ABW-2732-fix-transaction-preview-scroll branch from 28e7c4d to 3cdd04d Compare February 1, 2024 10:35
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

@giannis-rdx giannis-rdx merged commit e702c16 into main Feb 1, 2024
8 of 9 checks passed
@giannis-rdx giannis-rdx deleted the bugfix/ABW-2732-fix-transaction-preview-scroll branch February 1, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakub-rdx jakub-rdx jakub-rdx approved these changes

@raf-rdx raf-rdx raf-rdx approved these changes

@micbakos-rdx micbakos-rdx Awaiting requested review from micbakos-rdx

Assignees

@giannis-rdx giannis-rdx

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@giannis-rdx @micbakos-rdx @raf-rdx @jakub-rdx