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

Fixed non scrollable wallet screen #2758

Conversation

scoder1747
Copy link
Contributor

@scoder1747 scoder1747 commented Dec 4, 2024

Summary

Related to Issue: #2744

In wallet nwc screen, the user cannot scroll to bottom. The screenshot of the issue is below.

Description of Image

Video showing the scrollable nwc screen after applying the fix

ScreenRecording_12-04-2024.00-25-24_1.mov

Description of resolution:

The views were added inside ScrollView.

Demo after PR updates (Vstack spacing)-

iPhone 16:

Description of Image

iPhone SE:

Description of Image

Checklist

  • I have read (or I am familiar with) the Contribution Guidelines
  • I have tested the changes in this PR
  • My PR is either small, or I have split it into smaller logical commits that are easier to review
  • I have added the signoff line to all my commits. See Signing off your work
  • I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
    • I do not need to add a changelog entry. Reason: [Please provide a reason]
  • I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches

Test report

Please provide a test report for the changes in this PR. You can use the template below, but feel free to modify it as needed.

Device: [Please specify the device you used for testing]

iOS: [Please specify the iOS version you used for testing]

Damus: [Please specify the Damus version or commit hash you used for testing]

Setup: [Please provide a brief description of the setup you used for testing, if applicable]

Steps: [Please provide a list of steps you took to test the changes in this PR]

Results:

  • PASS
  • Partial PASS
    • Details: [Please provide details of the partial pass]

Other notes

[Please provide any other information that you think is relevant to this PR.]

@jb55
Copy link
Collaborator

jb55 commented Dec 4, 2024

looks good for now but we should look into moving this elsewhere

@alltheseas alltheseas linked an issue Dec 4, 2024 that may be closed by this pull request
@danieldaquino
Copy link
Contributor

Thank you @scoder1747, the code and the demo looks good!

However, for some reason the padding above and below the support section seems to be missing when I run it on my end. It does not look like the one shown in the demo:

Screenshot 2024-12-11 at 08 36 02

Damus: 13d9679329c26dd2fbe9f7b6e6e2244ca3f09a5f
iOS: 18.1
Device: iPhone 16 Pro and iPhone SE (simulators)

Can you please check?

@scoder1747 scoder1747 force-pushed the fix-non-scrollable-wallet-connect-screen branch from 13d9679 to 0acb8a2 Compare December 11, 2024 14:00
@scoder1747
Copy link
Contributor Author

scoder1747 commented Dec 11, 2024

Thank you @scoder1747, the code and the demo looks good!

However, for some reason the padding above and below the support section seems to be missing when I run it on my end. It does not look like the one shown in the demo:

Screenshot 2024-12-11 at 08 36 02 **Damus:** `13d9679329c26dd2fbe9f7b6e6e2244ca3f09a5f` **iOS:** 18.1 **Device:** iPhone 16 Pro and iPhone SE (simulators)

Can you please check?

Thanks for the feedback. I added the padding at the last moment but somehow forgot to stage the change. I have uploaded the change now. Demo video remains intact.

@scoder1747
Copy link
Contributor Author

scoder1747 commented Dec 11, 2024

Thank you @scoder1747, the code and the demo looks good!
However, for some reason the padding above and below the support section seems to be missing when I run it on my end. It does not look like the one shown in the demo:
Screenshot 2024-12-11 at 08 36 02
Damus: 13d9679329c26dd2fbe9f7b6e6e2244ca3f09a5f iOS: 18.1 Device: iPhone 16 Pro and iPhone SE (simulators)
Can you please check?

Thanks for the feedback. I added the padding at the last moment but somehow forgot to stage the change. I have uploaded the change now. Demo vides remains intact.

Screenshot below-

IMG_620B630F6CF3-1

@danieldaquino
Copy link
Contributor

Thank you @scoder1747!

I gave it a try, and it does look like the bottom padding improved since the last version, but it still does not match your screenshot for some reason, it looks like the top and bottom padding is set to zero on my end:

Damus: 0acb8a240e54d02d32e044680e29bc22692b78f5
iOS: 18.1
Device: iPhone 16 and SE (simulator)

Screenshot 2024-12-13 at 21 18 57

Can you please check what is causing this?

Thank you!

@scoder1747 scoder1747 force-pushed the fix-non-scrollable-wallet-connect-screen branch from 0acb8a2 to cc177ad Compare December 13, 2024 15:21
@scoder1747
Copy link
Contributor Author

Thank you @scoder1747!

I gave it a try, and it does look like the bottom padding improved since the last version, but it still does not match your screenshot for some reason, it looks like the top and bottom padding is set to zero on my end:

Damus: 0acb8a240e54d02d32e044680e29bc22692b78f5 iOS: 18.1 Device: iPhone 16 and SE (simulator)

Screenshot 2024-12-13 at 21 18 57 Can you please check what is causing this?

Thank you!

Hi @danieldaquino, thanks for the review again. I was able to see the padding issue in the device you've posted. Therefore, I ran on those devices and was able to address the issue. I am able to address by using Vstack spacing which will be applied the same way to all of the three sections in the view. Wallet Relay and Wallet Address components are inserted into one Vstack internally as per older implementation having it's own internal spacing (which I have not made any modification there in). I have provided the screenshots in the PR description for your review. Thanks!

@scoder1747 scoder1747 force-pushed the fix-non-scrollable-wallet-connect-screen branch from cc177ad to b7b1f4a Compare December 13, 2024 15:32
@danieldaquino
Copy link
Contributor

Thank you @scoder1747, The bottom padding problem is resolved on my end, however the top padding unfortunately still looks to be zero

Screenshot 2024-12-16 at 17 44 33

I believe the VStack spacing only applies in between stack objects, but not around the stack itself.

I took a closer look at the code, and this diff seems to solve the issue:

diff --git a/damus/Views/Wallet/WalletView.swift b/damus/Views/Wallet/WalletView.swift
index 1320d38a..949f9fb6 100644
--- a/damus/Views/Wallet/WalletView.swift
+++ b/damus/Views/Wallet/WalletView.swift
@@ -23,7 +23,7 @@ struct WalletView: View {
             VStack(spacing: 35) {
                 if !damus_state.settings.nozaps {
                     SupportDamus
-                        .padding(.bottom, 20)
+                        .padding(.vertical, 20)
                 }
                 
                 VStack(spacing: 5) {
---
base-commit: b7b1f4a1c25dfb2ec14e48d5eb85ec051973a7ce

Then it looks better and more similar to the screenshot you presented:

Screenshot 2024-12-16 at 17 49 47

I can apply this diff on my end and merge the PR manually if it saves time, just let me know if that's ok with you!

Thanks!

Changelog-Fixed: Fix non scrollable wallet screen
Signed-off-by: Swift Coder <[email protected]>
@scoder1747 scoder1747 force-pushed the fix-non-scrollable-wallet-connect-screen branch from b7b1f4a to f18a0b6 Compare December 16, 2024 15:23
@scoder1747
Copy link
Contributor Author

Thank you @scoder1747, The bottom padding problem is resolved on my end, however the top padding unfortunately still looks to be zero

Screenshot 2024-12-16 at 17 44 33 I believe the VStack spacing only applies in between stack objects, but not around the stack itself.

I took a closer look at the code, and this diff seems to solve the issue:

diff --git a/damus/Views/Wallet/WalletView.swift b/damus/Views/Wallet/WalletView.swift
index 1320d38a..949f9fb6 100644
--- a/damus/Views/Wallet/WalletView.swift
+++ b/damus/Views/Wallet/WalletView.swift
@@ -23,7 +23,7 @@ struct WalletView: View {
             VStack(spacing: 35) {
                 if !damus_state.settings.nozaps {
                     SupportDamus
-                        .padding(.bottom, 20)
+                        .padding(.vertical, 20)
                 }
                 
                 VStack(spacing: 5) {
---
base-commit: b7b1f4a1c25dfb2ec14e48d5eb85ec051973a7ce

Then it looks better and more similar to the screenshot you presented:

Screenshot 2024-12-16 at 17 49 47 I can apply this diff on my end and merge the PR manually if it saves time, just let me know if that's ok with you!

Thanks!

Thanks @danieldaquino. I have uploaded the necessary change and attached the screenshot below

Simulator Screenshot - iPhone 16 Pro Max - 2024-12-16 at 10 18 39

@danieldaquino danieldaquino merged commit 19e312a into damus-io:master Dec 18, 2024
@danieldaquino
Copy link
Contributor

Thank you @scoder1747! Approved and merged!

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

Successfully merging this pull request may close these issues.

Cannot scroll to bottom of NWC screen
3 participants