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 fullscreen view to group overview page #1435

Merged

Conversation

krauselukas
Copy link
Contributor

No description provided.

@@ -123,6 +126,8 @@ sub group_overview {
$self->stash('only_tagged', $only_tagged);
$self->stash('comments', \@comments);
$self->stash('pinned_comments', \@pinned_comments);
$self->stash('latest_comment', $latest_comment);
$self->stash('show_comments', $show_comments);
Copy link
Member

Choose a reason for hiding this comment

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

As this is the only use of these variables I suggest to directly query the params method here, i.e.

$self->stash('latest_comment', $self->param('latest_comment'));
…

Copy link
Member

Choose a reason for hiding this comment

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

stash is not needed if the unique purpose is to use it in the template

<button type="submit" class="btn btn-default">Apply</button>
</form>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to focus on one thing at a time in this PR, e.g. either full screen or "show comments" but the general idea is very much appreciated. But keep in mind that we have now a "limit to …" line just below the build bars and this dynamic filter box further below the comments. Probably one of two approaches should be followed consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@okurz Allright, totally overlooked it, thanks for the hint.

@codecov
Copy link

codecov bot commented Aug 21, 2017

Codecov Report

Merging #1435 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1435      +/-   ##
==========================================
+ Coverage   87.52%   87.52%   +<.01%     
==========================================
  Files         105      105              
  Lines        7918     7919       +1     
==========================================
+ Hits         6930     6931       +1     
  Misses        988      988
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/Main.pm 100% <100%> (ø) ⬆️

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 9fa6353...1a306a2. Read the comment docs.

@krauselukas krauselukas force-pushed the feature/fullscreen_group_overview branch from aa36f4a to 300072e Compare August 21, 2017 12:34
@krauselukas krauselukas force-pushed the feature/fullscreen_group_overview branch from 300072e to e942d6e Compare August 21, 2017 12:36
@@ -0,0 +1,27 @@
function hideNavbar(fullscreen) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since i'm using this function related to fullscreen on more than one place, i thought it would be better to make a seperate file for it

@@ -7,3 +7,12 @@
% my $selected = $rtagged{$only_tagged};
%= b join(' / ', map { $_ eq $selected ? "<b>$_</b>" : link_to($_ => url_with->query([only_tagged => $tagged{$_}])) } reverse sort keys %tagged);
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@okurz after you pointed out the "limit to" section, i made the full screen mode part of it
screenshot_2017-08-21_14-41-18

@krauselukas
Copy link
Contributor Author

I rewinded the comments part, and will make a seperate PR for it

@krauselukas krauselukas force-pushed the feature/fullscreen_group_overview branch from 5c63ea5 to 1a306a2 Compare August 21, 2017 14:26
@krauselukas krauselukas changed the title WIP: Add fullscreen view to group overview page Add fullscreen view to group overview page Aug 21, 2017
@sysrich sysrich merged commit 1a306a2 into os-autoinst:master Sep 4, 2017
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