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

Fixes #631 Scroll & constraint issues in BrowserViewController #894

Merged
merged 14 commits into from Apr 11, 2019

Conversation

@danishjafri88
Copy link
Contributor

danishjafri88 commented Feb 12, 2019

This is a regression PR for #631. Previous PR was reverted because of regression bug relating to constraints not properly set when bottom Toolbar is not visible.

Apart for the above regression fix, following are 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.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Reviewer Checklist:

  • PR is linked to an issue via Zenhub.
  • Issues are assigned to at least one epic.
  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable)
danishjafri88 added 10 commits Jan 2, 2019
…eaderTopOffset` is already clamped by `-topScrollHeight` in scrollWithDelta, where topScrollHeight contains both urlView and tabsBarView.
…o treating it as non-nil while declaring it as unwrapped optional. Removed old animation code fix which became redundant after update
@danishjafri88 danishjafri88 requested review from jhreis and kylehickinson Feb 12, 2019
@danishjafri88 danishjafri88 changed the title Fixes #631 Fixes #631 Scroll & constraint issues in BrowserViewController Feb 12, 2019
@jhreis jhreis removed the QA/Yes label Mar 21, 2019
Copy link
Contributor

jhreis left a comment

This feels much better to me, really solid improvements IMO, hopefully others will agree! Just one block of code/comments to remove.

@@ -251,20 +244,25 @@ private extension TabScrollingController {
}

func animateToolbarsWithOffsets(_ animated: Bool, duration: TimeInterval, headerOffset: CGFloat, footerOffset: CGFloat, alpha: CGFloat, completion: ((_ finished: Bool) -> Void)?) {
//NOTE: The issue below (& its fix) is irrelevant now since the the scroll logic is updated, infact the logic further

This comment has been minimized.

Copy link
@jhreis

jhreis Apr 11, 2019

Contributor

Just delete all of this, no reason to keep old code laying around.

@jhreis
jhreis approved these changes Apr 11, 2019
@jhreis jhreis merged commit cefd069 into development Apr 11, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhreis jhreis deleted the bugfix/fix.bug.631 branch Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.