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

Logs Auto Scroll #3561

Merged
merged 9 commits into from
Feb 26, 2019
Merged

Conversation

eloyekunle
Copy link
Contributor

@eloyekunle eloyekunle commented Feb 11, 2019

Update the Logs view to support autoscroll when Logs refresh.

Closes #2610

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 11, 2019
@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #3561 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3561      +/-   ##
==========================================
+ Coverage   48.38%   48.42%   +0.03%     
==========================================
  Files         165      165              
  Lines        8000     8000              
  Branches       24       24              
==========================================
+ Hits         3871     3874       +3     
+ Misses       3862     3858       -4     
- Partials      267      268       +1
Impacted Files Coverage Δ
src/app/backend/sync/secret.go 73.26% <0%> (+2.97%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a8eb99...9c7e1e3. Read the comment docs.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 11, 2019
@eloyekunle eloyekunle changed the title [WIP] - Logs Auto Scroll Logs Auto Scroll Feb 11, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2019
@eloyekunle
Copy link
Contributor Author

/assign maciaszczykm

@maciaszczykm
Copy link
Member

It does not solve #3509. The original issue with more context is #1083. I will take a look tomorrow.

Copy link
Member

@jeefy jeefy left a comment

Choose a reason for hiding this comment

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

Agreeing with @maciaszczykm -- This doesn't solve #3509, it only solves #2610

I'd lgtm it once we clarify that auto-scrolling was the only intent for this PR.

Thanks!

@maciaszczykm
Copy link
Member

Works fine for page refresh, but not when switching pages. It is quote uncomfortable at the moment, but newest logs are at the end. Can you scroll also on page changes?

And remove #3509 from fixed issues please.

@eloyekunle
Copy link
Contributor Author

I've removed #3509, I'll have to thrash that on another PR.

The current behavior is it scrolls only when the user is already at the bottom of the logs viewer. This is to avoid a situation where you're trying to read something higher up in the page but the logs keep scrolling down during auto-refresh.

Currently, when you switch pages, you arrive at the top of the target page so it doesn't scroll because you're not at the bottom.

What do you think?

@floreks
Copy link
Member

floreks commented Feb 14, 2019

I'd say that on page change to next one we should stay on top, but on page change to the previous one, we should go to the bottom to make it feel like you can continue reading without scrolling manually.

As for the autoscroll to bottom when using autorefresh, I would maybe add new option to follow the logs. Maybe something like travis does?

@eloyekunle
Copy link
Contributor Author

eloyekunle commented Feb 14, 2019

Okay, new changes:

  • Navigation to previous pages should go to the bottom.
  • Add new option to follow logs.

Sounds good.

@jeefy
Copy link
Member

jeefy commented Feb 16, 2019

Also it looks like you have a conflict in src/app/frontend/logs/template.html

When you make your additional changes, please resolve that before pushing. :)

Thanks!

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 19, 2019
@eloyekunle
Copy link
Contributor Author

Implemented:

  • Option to follow logs. This adds a new icon.
  • Scroll to logs bottom when navigating to previous page.
  • Scroll to logs top when navigation to next page.

@jeefy
Copy link
Member

jeefy commented Feb 21, 2019

@eloyekunle Your latest changes broke some HTML formatting.

See https://travis-ci.org/kubernetes/dashboard/jobs/496191334#L297

You can test these CI checks locally by running npm run check (and others, see package.json for the other options)

After that quick formatting fix I'll give it one more look and lgtm it. :) Thanks!

@eloyekunle
Copy link
Contributor Author

@jeefy Thanks, I've fixed the HTML formatting.

eloyekunle and others added 5 commits February 21, 2019 10:09
- scrolls to bottom on navigation to prev page
- scrolls to top on navigation to next page
@jeefy
Copy link
Member

jeefy commented Feb 26, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eloyekunle, jeefy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3f1009e into kubernetes:master Feb 26, 2019
@eloyekunle eloyekunle deleted the cleanup/logs-auto-scroll branch February 27, 2019 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants