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 silent reload of dashboard contents for fullscreen mode #2684

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

asdil12
Copy link
Member

@asdil12 asdil12 commented Jan 22, 2020

This will allow setting the get parameter interval to a value of seconds after which the index page is reloaded - but without loading indicator to always show results.
This is intended for dashboard usage.

@asdil12 asdil12 marked this pull request as ready for review January 22, 2020 11:02
@asdil12 asdil12 requested a review from Martchus January 22, 2020 11:02
@Martchus
Copy link
Contributor

The commit message "Silent index autoreload of for dashboard use" is not nice. I would it rephrase as "Add silent reload of dashboard contents for fullscreen mode".

@kalikiana kalikiana changed the title Silent index autoreload of for dashboard use Add silent reload of dashboard contents for fullscreen mode Jan 22, 2020
@okurz
Copy link
Member

okurz commented Jan 22, 2020

Would be nice but checks in OBS fail currently. We should only merge on green test results

@okurz
Copy link
Member

okurz commented Jan 22, 2020

Could you please use an approach that is compatible with 2b3885a as described in https://progress.opensuse.org/issues/17886 so that we do not have two refresh features?

@asdil12
Copy link
Member Author

asdil12 commented Jan 23, 2020

I don's see how that code could be reused in a valuable way. It would only create spaghetty code to mix index.js and fullscreen.js.
I had a talk to Marius and it would make more sence to rework the code from 2b3885a.
Also I made sure to use the same get parameter interval so that it will be transparent to the user.

@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #2684 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2684      +/-   ##
==========================================
- Coverage   91.94%   91.83%   -0.11%     
==========================================
  Files         184      184              
  Lines       11589    11589              
==========================================
- Hits        10655    10643      -12     
- Misses        934      946      +12
Impacted Files Coverage Δ
lib/OpenQA/Worker/Settings.pm 84.61% <0%> (-13.47%) ⬇️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 87.69% <0%> (-1.54%) ⬇️

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 75fc711...8e1c2d1. Read the comment docs.

@asdil12
Copy link
Member Author

asdil12 commented Jan 23, 2020

Rebased to current master to retrigger testsuite.

@Martchus
Copy link
Contributor

To reuse or merge with existing auto-refresh code we needed to port the group overview to using AJAX first. Both auto-refresh approaches are just a few lines of code so I wouldn't overthink it.

@okurz
Copy link
Member

okurz commented Jan 23, 2020

sure, I agree. However I think the parameter is quite hidden. Can we add it to some help-text on the page?

@Martchus
Copy link
Contributor

Yes, it is hidden. A better alternative would be adding a from element (like we already have for the fullscreen parameter) with a help-text. If it is good enough for @asdil12 as it is I wouldn't make that improvement mandatory for merging the PR.

Note that the fullscreen and interval parameters are so far also completely hidden on the group overview and the timeago rendering is broken on the group overview when setting the interval. So there's generally room for improvement.

@okurz
Copy link
Member

okurz commented Jan 23, 2020

fine. Martchus will cover the missing steps in the mentioned ticket.

@okurz okurz merged commit 055c8e2 into os-autoinst:master Jan 23, 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

4 participants