499 Inconsistent padding #596

jeden · 2024-11-28T00:40:12Z

Related Issue

Ticket: Inconsistent padding #499
Follow-up: [TECH-DEBT] Refactor pull-up sheets to use dynamic height #647

Summary of Changes

Note: in this PR the min iOS version has been increased to 16.

Some implementation details:

iOS automatically chooses how to position the visible area when the keyboard is visible. If there's enough real estate, the bottom button is completely visible, positioned right above the keyboard. In case of not enough space, it seems to keep the edited text field at the vertical center of the visible area above the keyboard.
Adding a scroll view (with or without a geometry reader) invalidates the height calculation and causes the sheet view to not be displayed at all (except for a small frame at the bottom). If the view is not scrollable it works fine. This shouldn't be a major issue though, with larger text the bottom button might go out of the visible space, and so does the title area, but the text edit should still remain visible.

Only the halfSheet() modifier, and corresponding places where it is used, have been refactored.

PresentHostingController and CustomHostingController require additional refactoring that will be addressed on a separate ticket (#647)

Need Regression Testing

Yes: all UI places where pull-up sheets are displayed
No

Risk Assessment

Low
Medium: any implementation mistake can lead to incorrect layouts that might affect UX
High

Additional Notes

Screenshots (if applicable)

See this comment for a list of screenshots where the half sheet is used.

#499 - Temporary implementation

#499

# Conflicts: # FRW/Modules/Wallet/MoveAsset/MoveTokenView.swift

jeden · 2024-11-28T00:41:35Z

FRW/Modules/Wallet/MoveAsset/MoveComponentView.swift

+                .lineLimit(nil)
+                .fixedSize(horizontal: false, vertical: true)


This enables the text to be displayed in multiple lines, if needed, otherwise it stays on a single line and wrapped if necessary.

zhouxl · 2024-11-28T01:15:04Z

custom height is only on iOS 16 and up？

lmcmz · 2024-12-01T22:45:16Z

Can we have a horizontal solution cross the app ?
Cause this is no an issue on this page only, We have many place need adjust dynamic height.

lmcmz · 2024-12-01T23:33:08Z

Meanwhile, when I put more elements in this bottom sheet, it doesn't dynamic adjust the content height.

jeden · 2024-12-01T23:57:46Z

custom height is only on iOS 16 and up？

@zhouxl I thought I had answered to this question... maybe it was in the other PR that I closed.

Anyway yes, unfortunately this works on iOS 16+ only

jeden · 2024-12-02T00:00:16Z

Can we have a horizontal solution cross the app ? Cause this is no an issue on this page only, We have many place need adjust dynamic height.

@lmcmz Yes, the implementation is reusable, but it requires some minor refactoring on a case-by-case basis, similar to what done in MoveTokenView.swift

jeden · 2024-12-02T00:01:31Z

Meanwhile, when I put more elements in this bottom sheet, it doesn't dynamic adjust the content height.
@lmcmz
What have you added? Can you share the code so I can take a look?

jeden · 2024-12-02T00:36:07Z

Meanwhile, when I put more elements in this bottom sheet, it doesn't dynamic adjust the content height.
@lmcmz
What have you added? Can you share the code so I can take a look?

nvm... I see you've duplicated the source-target row. I'll check that out

jeden · 2024-12-02T03:50:56Z

@lmcmz what device and iOS version are you on?

This is taken from the iPhone 15 Pro (simulator), iPhone SE (simulator):

And this from my iPhone 15 Pro (device)

lmcmz · 2024-12-02T13:20:53Z

This move token view can be init from multiple places. The one I tested from home screen doesn't work.

I would suggest we apply this dynamic height configuration in all places using

CustomHostingController
halfSheet
PresentHostingController

lmcmz · 2024-12-02T13:23:46Z

Meanwhile, I can see this bottom sheet have a transparent background on bottom safe area. We need fix this.

Probably, we fix on different PR, the insufficient storage alert shown't overlap with the move fee UI, it was overlap due to the height is not dynamic.

jeden · 2024-12-02T22:37:10Z

This move token view can be init from multiple places. The one I tested from home screen doesn't work.

Can we meet today so you can show me in which case it does not work? Because I am unable to reproduce that.

I would suggest we apply this dynamic height configuration in all places using
CustomHostingController
halfSheet
PresentHostingController

I suggest deferring this to a separate ticket, to prevent this from being out of sync with develop again.

jeden · 2024-12-02T22:39:30Z

Meanwhile, I can see this bottom sheet have a transparent background on bottom safe area. We need fix this.

Probably, we fix on different PR, the insufficient storage alert shown't overlap with the move fee UI, it was overlap due to the height is not dynamic.

The button is on an overlay, so it doesn't contribute to the underlying view's height — and the alert is displayed in the same overlay (because it's linked to the button). In order for that to happen I should move the button from the overlay to the view.

…s to the view height

lmcmz · 2024-12-04T00:07:01Z

It doesn't look good when there is a keyboard

jeden · 2024-12-04T00:29:33Z

Yeah I haven't reworked on that after the discussions of the past couple days

- Increased the minimum target to 16.0

lmcmz · 2024-12-09T13:00:05Z

Not sure, why we are not continue working on this.
We need meet following requirements:

Adapted for keyboard input scenarios
The bottom safe area has no background
We need apply this auto resize to all our bottom sheet popup.

jeden · 2024-12-09T19:27:41Z

This is what I have been working on in the latest days. I haven't pushed any commit since I needed to fix the layout issues first and then check and refactor all places where the half sheet is used.

I should resubmit this for review today or tomorrow. However I think I'll defer the other 2 usages (PresentHostingController and CustomHostingController) to a separate ticket, because this is diverging too much from dev and merging might become more complicated the more I wait.

jeden · 2024-12-09T22:33:44Z

backup tips shown on home

settings -> devices -> device info -> key location -> swipe left -> tap "Revoke":

home -> plus button -> tap any circled plus button

settings -> backup -> multi-backup -> swipe left any backup -> tap "Delete"

settings -> linked accounts -> any child account -> tap "Unlink Account"

settings -> backup -> multi-backup -> tap any -> tap "Delete Backup"

NFT -> tap plus button -> any NFT, tap circular plus button

NFT -> tap any NFT -> tap the "Move" button

home -> tap "Stake" button -> tap "Stake New Node" -> tap any token -> input amount -> tap "Next"

home -> tap any token -> tap swap button -> type amount -> select destination token -> tap "Swap beta"

home -> tap any token -> tap "move" button

home -> tap any token -> tap send button -> select recipient -> type amount -> tap "Next"

# Conflicts: # FRW.xcodeproj/project.pbxproj # FRW/Resource/en.lproj/Localizable.strings

jeden added 3 commits November 27, 2024 04:09

499 Inconsistent padding

5860629

#499 - Temporary implementation

499 Inconsistent padding

9dc73ad

#499

Merge branch 'develop' into fts/499-inconsistent-padding-v2

487665a

# Conflicts: # FRW/Modules/Wallet/MoveAsset/MoveTokenView.swift

jeden commented Nov 28, 2024

View reviewed changes

jeden linked an issue Nov 29, 2024 that may be closed by this pull request

Inconsistent padding #499

Closed

- Moved button from overlay to inside the view, so that it contribute…

2a88b4c

…s to the view height

jeden added the blocked label Dec 4, 2024

jeden marked this pull request as draft December 4, 2024 00:30

jeden added 2 commits December 5, 2024 01:09

- Fixed animations, safe areas

d4582ea

- Increased the minimum target to 16.0

- refactored and checked all sheets using the half-sheet modifier

eaad21b

jeden added 3 commits December 9, 2024 23:42

- Final adjustments

e419f10

- Removed conditional code targeting iOS below 16

2374079

Merge branch 'develop' into fts/499-inconsistent-padding-v2

d684eff

# Conflicts: # FRW.xcodeproj/project.pbxproj # FRW/Resource/en.lproj/Localizable.strings

jeden marked this pull request as ready for review December 10, 2024 00:43

jeden removed the blocked label Dec 10, 2024

jeden requested review from zhouxl and lmcmz and removed request for zhouxl December 10, 2024 00:48

lmcmz approved these changes Dec 10, 2024

View reviewed changes

jeden merged commit 06585d6 into develop Dec 10, 2024
1 check passed

jeden deleted the fts/499-inconsistent-padding-v2 branch December 10, 2024 22:46

jeden mentioned this pull request Dec 10, 2024

Inconsistent padding #499

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

499 Inconsistent padding #596

499 Inconsistent padding #596

jeden commented Nov 28, 2024 •

edited

Loading

jeden Nov 28, 2024

zhouxl commented Nov 28, 2024

lmcmz commented Dec 1, 2024

lmcmz commented Dec 1, 2024

jeden commented Dec 1, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024 •

edited

Loading

lmcmz commented Dec 2, 2024

lmcmz commented Dec 2, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024

lmcmz commented Dec 4, 2024

jeden commented Dec 4, 2024

lmcmz commented Dec 9, 2024

jeden commented Dec 9, 2024

jeden commented Dec 9, 2024 •

edited

Loading

		.lineLimit(nil)
		.fixedSize(horizontal: false, vertical: true)

499 Inconsistent padding #596

499 Inconsistent padding #596

Conversation

jeden commented Nov 28, 2024 • edited Loading

Related Issue

Summary of Changes

Need Regression Testing

Risk Assessment

Additional Notes

Screenshots (if applicable)

jeden Nov 28, 2024

Choose a reason for hiding this comment

zhouxl commented Nov 28, 2024

lmcmz commented Dec 1, 2024

lmcmz commented Dec 1, 2024

jeden commented Dec 1, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024 • edited Loading

lmcmz commented Dec 2, 2024

lmcmz commented Dec 2, 2024

jeden commented Dec 2, 2024

jeden commented Dec 2, 2024

lmcmz commented Dec 4, 2024

jeden commented Dec 4, 2024

lmcmz commented Dec 9, 2024

jeden commented Dec 9, 2024

jeden commented Dec 9, 2024 • edited Loading

jeden commented Nov 28, 2024 •

edited

Loading

jeden commented Dec 2, 2024 •

edited

Loading

jeden commented Dec 9, 2024 •

edited

Loading