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 autorefresh in the index page for fullscreen mode feature #3141

Merged

Conversation

ilausuch
Copy link
Contributor

@ilausuch ilausuch commented Jun 1, 2020

https://progress.opensuse.org/issues/17886

This new feature allows to autorefresh the index page with 30, 60,
120 and 180 seconds. The user controls the interval with the
filter control panel.

@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #3141 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3141   +/-   ##
=======================================
  Coverage   91.89%   91.90%           
=======================================
  Files         211      211           
  Lines       12921    12924    +3     
=======================================
+ Hits        11874    11878    +4     
+ Misses       1047     1046    -1     
Impacted Files Coverage Δ
lib/OpenQA/Worker/Job.pm 75.03% <100.00%> (+0.11%) ⬆️
lib/OpenQA/WebAPI/Plugin/Helpers.pm 95.91% <0.00%> (+0.40%) ⬆️

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 cafc562...7bf5866. Read the comment docs.

@Martchus
Copy link
Contributor

Martchus commented Jun 2, 2020

Could you start the PR title and commit messages with a verb in imperative so it is clear whether this is a fix/extension/improvement of an existing feature or a new feature?

@ilausuch ilausuch force-pushed the index_auto_refresh_full_screen branch from dc21202 to b56464c Compare June 2, 2020 13:13
@ilausuch
Copy link
Contributor Author

ilausuch commented Jun 2, 2020

Also is fixed a bug in the code that didn't show the interval selector when refresh the page and the full screen mode was on

@ilausuch ilausuch changed the title Autorefresh in the index page Add autorefresh in the index page for fullscreen mode feature Jun 2, 2020
@ilausuch ilausuch marked this pull request as ready for review June 2, 2020 13:16
@ilausuch ilausuch force-pushed the index_auto_refresh_full_screen branch from b56464c to 871485b Compare June 3, 2020 08:44
templates/webapi/main/index.html.ep Outdated Show resolved Hide resolved
assets/javascripts/index.js Outdated Show resolved Hide resolved
assets/javascripts/index.js Outdated Show resolved Hide resolved
assets/javascripts/index.js Outdated Show resolved Hide resolved
assets/javascripts/index.js Outdated Show resolved Hide resolved
@ilausuch ilausuch force-pushed the index_auto_refresh_full_screen branch from 871485b to 4f65c51 Compare June 4, 2020 07:28
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I've tested it locally and the page now behaves rather weird. By default, the "Auto refresh" filter options are available and the fullscreen mode is disabled. I'd say that makes sense so far. The auto-reload might be used without the fullscreen mode. However, when toggling the fullscreen mode the "Auto refresh" filter options are toggled as well which doesn't make too much sense to me. It also seems like the auto refresh only works when the fullscreen mode is enabled.

The commit message is full of typos and grammar mistakes. Besides, it seems like it convers only to the CSS change.

@ilausuch
Copy link
Contributor Author

ilausuch commented Jun 5, 2020

Ok, I am removing the part that only allows auto-refresh in fullscreen mode

@okurz
Copy link
Member

okurz commented Jun 5, 2020

your tests fail now in the cache step as the versions of packages can not be found anymore, see https://app.circleci.com/pipelines/github/os-autoinst/openQA/3181/workflows/7eeb8bf3-ee48-433d-a525-943e06cc17a6/jobs/30369/steps

@kalikiana this makes me think that what is tested is actually not the to-be-merged state but pre-merge.

@ilausuch please try to fix that by rebasing on a current master with updated dependencies.txt

@ilausuch ilausuch force-pushed the index_auto_refresh_full_screen branch from 4f65c51 to c7f56cf Compare June 5, 2020 09:29
https://progress.opensuse.org/issues/17886

This new feature allows to autorefresh the index page with 30, 60,
120 and 180 seconds. The user controls the interval with the
filter control panel.
The autorefresh filter is only visible and applicable when
fullscreen is selected
Problem: When this page is in fullmode and the filter window is
closed (for instance if we comeback keeping the fullscreen=1
query value in url) is impossible to open the filter panel
because the header appears when the cursor enters in the filter
panel area to be clicked

Solution: Add additional margin to the top of the filter panel
@ilausuch ilausuch force-pushed the index_auto_refresh_full_screen branch from c7f56cf to 7bf5866 Compare June 5, 2020 09:31
@ilausuch
Copy link
Contributor Author

ilausuch commented Jun 5, 2020

Now is not necessary to be in a fullscreen mode to have auto-refresh. Also I rebased.
There are 4 commits, when you approve this, I'll do squash before merge

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

It should be good now. Of course the selection will not be shown accordingly when one manipulates the URL query manually but I guess it is ok. GitHub allows to squash commits before merging so you don't have to do that manually.

@okurz
Copy link
Member

okurz commented Jun 5, 2020

t/ui/16-tests_job_next_previous.t failed. I doubt your changes should influence that though. Please squash the commits and we will see new test results anyway

@Martchus
Copy link
Contributor

Martchus commented Jun 5, 2020

I've re-triggered only the failing UI tests. That should be faster. There's no need to squash commits locally anymore.

@Martchus Martchus merged commit 77bc64c into os-autoinst:master Jun 5, 2020
@ilausuch ilausuch deleted the index_auto_refresh_full_screen branch November 5, 2020 09:53
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