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

Add option for different ScrollPosition behavior on keyboard appearance #2112

Merged

Conversation

lcoderre
Copy link
Contributor

@lcoderre lcoderre commented Nov 2, 2020

Hello πŸ‘‹

Context
The default tableView.scrollTo with a ScrollPosition.none should animate in a way that the cell is fully visible. It works in most cases, but it seems to fail (scrolls out of view bounds) in the case of a cell preceding a very long footer. Most of the blame here is on Apple side, as I'll show below, however this change request could easily allow to bypass the issue.

Example/Experiment
In this commit (lcoderre@13c5d65) I modified the examples to add a very long Lorem Ipsum footer to a section, and also demonstrated using a pure UITableViewController with scrolling on every cell-click.

πŸ‘€ - You might notice I recorded this on iPhone 11 pro simulator, but I also repeated the test on iPhone 8, 6s. Same Same.

πŸ‘€ - Even a pure UITableViewController scrolls out of view for that last cell. And on the Eureka's Native Event UI, it acts a bit differently, but buggy all the same. It is worth noting that Eureka's scrolling behavior shown in the following gif isn't exactly the same as we're observing in our app using Eureka. In our case, it's more like Apple's version πŸ€·β€β™‚οΈ To be honest, I haven't dug in detail to find the reason.

See scrolling using .none
eureka-none-out

See now using .top (πŸ‘€ no bug)
eureka-top-out

Proposed solution
The proposed solution has the advantage of being "additive" and not "disruptive". I.e. it won't break anyone's code, but will allow some of us to bypass a nasty problem :)

⚠️ I have not considered extending the pattern to other places where there is scrolling, since I did not know if there was a need. Whereas in the context of keyboard showing up, making sure the cell with textfield was shown was my priority.

Copy link
Member

@mats-claassen mats-claassen left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation. Adding a parameter like this is always easy and just adds functionality. I think it might be desired at some point to define the scroll position on a per row basis but let's leave that for now

@mats-claassen mats-claassen merged commit 08574ae into xmartlabs:master Nov 3, 2020
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.

None yet

2 participants