Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFixed #631 Scroll issues. #670
Conversation
| } | ||
|
|
||
| didSet { | ||
| self.scrollView?.addGestureRecognizer(panGesture) | ||
| self.scrollView?.panGestureRecognizer.addTarget(self, action: #selector(handlePan)) | ||
| self.scrollView?.panGestureRecognizer.maximumNumberOfTouches = 1 |
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jan 2, 2019
Contributor
Should we be modifying the scroll view's pan gesture? This may have side-effects
This comment has been minimized.
This comment has been minimized.
danishjafri88
Jan 2, 2019
Author
Contributor
Agreed. I copied what was done for the PanGesture created. Looking at it now there is no need to update this value.
| @@ -667,8 +667,8 @@ class BrowserViewController: UIViewController { | |||
| webViewContainerTopOffset = make.top.equalTo(readerModeBar?.snp.bottom ?? self.header.snp.bottom).constraint | |||
|
|
|||
| let findInPageHeight = (findInPageBar == nil) ? 0 : UIConstants.ToolbarHeight | |||
| if let toolbar = self.toolbar { | |||
| make.bottom.equalTo(toolbar.snp.top).offset(-findInPageHeight) | |||
| if let footerView = self.footer { | |||
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jan 2, 2019
Contributor
Why does this work? footer will never be nil whereas toolbar will be when in landscape
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jan 2, 2019
Contributor
Sorry I didn't mean why do you need the if-let, I meant this will execute every time now and never hit the else below. footer is set and never un-set.
This comment has been minimized.
This comment has been minimized.
danishjafri88
Jan 2, 2019
Author
Contributor
Oh, so footer is constrained to bottom of view and when there is no toolbar its height becomes 0. Hence its constrained to bottom of screen. If footer will never be nil then perhaps we can remove the else case and also make it non-optional. What do you suggest?
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jan 7, 2019
•
Contributor
I'd suggest taking a closer look at why it used toolbar and if it really doesn't matter (like you said, its height becomes 0 anyways), then we can just remove the else clause here. In fact, we can also change the declaration of footer so its not implicitly unwrapped:
let footer = UIView()
rather than its current declaration: var footer: UIView! and being init'd later
This comment has been minimized.
This comment has been minimized.
danishjafri88
Jan 9, 2019
Author
Contributor
Kept it as unwrapped optional as it is initialised in viewdidload, but used without ? as it is never set to nil.
…m for WKWebview.
|
Not sure if you still want @jhreis to take a look, so delaying merge for now |
danishjafri88 commentedJan 2, 2019
•
edited
What is fixed:
->Youtube header problem
->Redundant Pan Gesture creation
->Mis-aligned constraints relations in BrowserViewController
->Refactored Show/Hide Tab Animation properly
-> Enable Zoom by ignoring viewport scale limit.
-> Removed a part of animation code(Hotfix for older logic) which became source of a jump issue during scroll.
Pull Request Checklist
My patch has gone through review and I have addressed review comments
My patch has a standard commit message that looks like
Fix #123: This fixes the shattered coffee cup!I have updated the Unit Tests to cover new or changed functionality
I have updated the UI Tests to cover new or changed functionality
I have marked the bug with
[needsuplift]I have made sure that localizable strings use
NSLocalizableString()Screenshots
If your patch includes user interface changes that you would like to suggest or that you would like UX to look at, please include them here.
Notes for testing this patch
If useful, please leave notes for QA, explaining what this patch changes and how it can be best tested and verified.