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

Jump to bottom

Only show scroll links when log is offscreen #5614

Merged
merged 1 commit into from Nov 3, 2015
Merged

Only show scroll links when log is offscreen #5614

merged 1 commit into from Nov 3, 2015

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Nov 2, 2015

Fixes #5566

@spadgett
Copy link
Member Author

spadgett commented Nov 2, 2015

@jwforres please review

@spadgett spadgett force-pushed the follow-link branch 2 times, most recently from 674f101 to ac25805 Compare November 2, 2015 23:33
@jwforres
Copy link
Member

jwforres commented Nov 2, 2015

Since you are using some native DOM checks, did you check both IE 10 and IE 11 just to be safe

@spadgett
Copy link
Member Author

spadgett commented Nov 2, 2015

I tested IE 11. Let me check on 10.

@spadgett
Copy link
Member Author

spadgett commented Nov 2, 2015

Having trouble with my IE 10 environment, but MDN says they're supported since IE 6.

https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect
https://developer.mozilla.org/en-US/docs/Web/API/Element/clientHeight

@spadgett spadgett mentioned this pull request Nov 3, 2015
Closed
@jwforres
Copy link
Member

jwforres commented Nov 3, 2015

LGTM
@openshift/team-project-committers fixes some usability weirdness with the appearance of follow log / top of log links, if any part of the log is off screen they will show up now

@spadgett
Copy link
Member Author

spadgett commented Nov 3, 2015

@jwforres added an extra set of parens around the or condition in updateScrollLinks

@jwforres
Copy link
Member

jwforres commented Nov 3, 2015

LGTM, just needs approval

@smarterclayton smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2015
@smarterclayton
Copy link
Contributor

Approved, relatively low risk

@jwforres
Copy link
Member

jwforres commented Nov 3, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3918/) (Image: devenv-rhel7_2636)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to a0426ad

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@spadgett
Copy link
Member Author

spadgett commented Nov 3, 2015

[test]

1 similar comment
@jwforres
Copy link
Member

jwforres commented Nov 3, 2015

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to a0426ad

openshift-bot pushed a commit that referenced this pull request Nov 3, 2015
Merge pull request #5614 from spadgett/follow-link
668d8e4 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 668d8e4 into openshift:master Nov 3, 2015
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6716/)

@spadgett spadgett deleted the follow-link branch November 4, 2015 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@spadgett @jwforres @smarterclayton @openshift-bot