Bootstrap/monitor #6421

ChrisBr · 2018-11-30T15:45:51Z

The project build montior table is now a DataTable and
all filter / search is performed in the DataTable now
instead of on the Server.

The monitor action still allows server filtering by passing parameters.
We still need to support this because of existing links.
However, when we already filtered on server side, the rendered table would
only contain a the filtered list which causes the client search to not
work as expected.

For now, we just disable the client search in this case and show a button
to remove all server side filters. This does a simple roundtrip to the server
without parameters which enables us to activate the client search again.

@Ana06 regarding revert of 3e45c4b, it does hurt, see screenshot 😄

obs-bot · 2018-11-30T15:51:18Z

Review app will appear here: http://obs-reviewlab.opensuse.org/chrisbr-bootstrapmonitor

ChrisBr · 2018-11-30T16:18:47Z

You can try it out here:

https://obs-reviewlab.opensuse.org/chrisbr-bootstrapmonitor/project/monitor/home:Admin

And with the serverside filter here:

https://obs-reviewlab.opensuse.org/chrisbr-bootstrapmonitor/project/monitor/home:Admin?pkgname=ctris

dmarcoux

Nice work! One question, why do we have this gap in the datatable?

Here's what I mean as highlighted in the screenshot below:

src/api/app/assets/javascripts/webui2/project_monitor.js

src/api/app/views/webui2/webui/project/_monitor_control.html.haml

src/api/app/views/webui2/webui/project/monitor.html.haml

src/api/app/assets/javascripts/webui2/project_monitor.js

src/api/app/views/webui2/webui/project/monitor.html.haml

Ana06 · 2018-12-03T09:53:44Z

src/api/app/views/webui2/webui/project/monitor.html.haml

+                %td
+                  = link_to word_break(packname, 40), :controller => "package", :action => "show", :package => packname, :project => @project.to_s
+                - @repohash.sort.each do |repo, archlist|
+                  - archlist.sort.each do |arch|


isn't this already sorted? 🤔

Apparently not, no

I think it comes sorted from the backend.... 🤔

How do you know? I would keep it for now, that is c&p from the old view.

src/api/app/views/webui2/webui/project/_monitor_control.html.haml

src/api/app/assets/stylesheets/webui2/webui2.css.scss

ChrisBr · 2018-12-03T10:29:17Z

Nice work! One question, why do we have this gap in the datatable?

This is the https://datatables.net/reference/option/scrollCollapse option. The reason is that otherwise the page size will jump all the time which is also not a nice experience. But I don't care too much, if you would like to collapse it, I'm fine with it too.

dmarcoux · 2018-12-03T10:52:24Z

@ChrisBr I think I would prefer to collapse it.

bgeuken · 2018-12-03T14:32:48Z

Nice work 👍

Two things I noticed:

When accessing the monitor page via a link (pre-filtered), e.g. via the build result links, the filters are not shown, but there is a flash notification.
The legend is a bit narrow and long. I think by making it wider we can make better use of the space.

ChrisBr · 2018-12-03T14:47:34Z

When accessing the monitor page via a link (pre-filtered), e.g. via the build result links, the filters are not shown, but there is a flash notification.

@bgeuken that is correct as then the result is already prefiltered on the server, allowing client side search does not make much sense then. In this case we add a button to remove the filter. See my description:

The monitor action still allows server filtering by passing parameters.
We still need to support this because of existing links.
However, when we already filtered on server side, the rendered table would
only contain a the filtered list which causes the client search to not
work as expected.

For now, we just disable the client search in this case and show a button
to remove all server side filters. This does a simple roundtrip to the server
without parameters which enables us to activate the client search again.

ChrisBr · 2018-12-03T14:48:27Z

The legend is a bit narrow and long. I think by making it wider we can make better use of the space.

This seems like a good idea.

ChrisBr · 2018-12-03T15:48:35Z

@dmarcoux here is a comparison when scrollCollapse is enabled:

Disabled:

vs.

Enabled:

As you can see, the whole page is basically jumping which adds a lot of noise. I don't like it to be honest.

dmarcoux · 2018-12-03T15:51:58Z

It's clearly better to have scrollCollapse disabled. Thank you for the comparison GIFs.

bgeuken

Nice work 👍

ChrisBr · 2018-12-04T13:49:32Z

I refactored the feature specs to use page objects:
3f8ed74

In case somebody is interested to read more about that, here are two nice resources:

Ana06

As already said, you need the .obs-dataTable class to solve the bug

The project build montior table is now a DataTable and all filter / search is performed in the DataTable now instead of on the Server. The monitor action still allows server filtering by passing parameters. We still need to support this because of existing links. However, when we already filtered on server side, the rendered table would only contain a the filtered list which causes the client search to not work as expected. For now, we just disable the client search in this case and show a button to remove all server side filters. This does a simple roundtrip to the server without parameters which enables us to activate the client search again.

The underlying issue got fixed in another way and this breaks other functionality of the modal. Co-authored-by: Ana María Martínez Gómez <[email protected]>

codecov · 2018-12-04T14:36:14Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #6421      +/-   ##
==========================================
+ Coverage    91.2%   91.21%   +<.01%     
==========================================
  Files         437      437              
  Lines       19668    19673       +5     
==========================================
+ Hits        17939    17944       +5     
  Misses       1729     1729

codecov · 2018-12-04T14:36:15Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #6421      +/-   ##
==========================================
+ Coverage    91.2%   91.21%   +<.01%     
==========================================
  Files         437      437              
  Lines       19668    19673       +5     
==========================================
+ Hits        17939    17944       +5     
  Misses       1729     1729

ChrisBr · 2018-12-04T15:15:36Z

coveralls hanging ...

ChrisBr added Frontend Things related to the OBS RoR app review-app Apply this label if you want a review app started Bootstrap 🚀 Bootstrap migration labels Nov 30, 2018

ChrisBr force-pushed the bootstrap/monitor branch from 25b9a70 to fb974ac Compare November 30, 2018 16:00

dmarcoux suggested changes Dec 3, 2018

View reviewed changes

Ana06 suggested changes Dec 3, 2018

View reviewed changes

ChrisBr force-pushed the bootstrap/monitor branch from fb974ac to 1775433 Compare December 3, 2018 15:41

dmarcoux approved these changes Dec 3, 2018

View reviewed changes

bgeuken approved these changes Dec 4, 2018

View reviewed changes

ChrisBr force-pushed the bootstrap/monitor branch from 1775433 to 3f8ed74 Compare December 4, 2018 13:47

ChrisBr force-pushed the bootstrap/monitor branch from 3f8ed74 to 39e24fc Compare December 4, 2018 13:50

Ana06 suggested changes Dec 4, 2018

View reviewed changes

ChrisBr and others added 3 commits December 4, 2018 15:21

Revert f898183

8f618ab

The underlying issue got fixed in another way and this breaks other functionality of the modal. Co-authored-by: Ana María Martínez Gómez <[email protected]>

Add feature specs for bootstrap monitor page

0386ddb

ChrisBr force-pushed the bootstrap/monitor branch from 39e24fc to 0386ddb Compare December 4, 2018 14:21

Ana06 approved these changes Dec 4, 2018

View reviewed changes

ChrisBr merged commit 2861dd2 into openSUSE:master Dec 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap/monitor #6421

Bootstrap/monitor #6421

ChrisBr commented Nov 30, 2018

obs-bot commented Nov 30, 2018

ChrisBr commented Nov 30, 2018

dmarcoux left a comment

Ana06 Dec 3, 2018

ChrisBr Dec 3, 2018

Ana06 Dec 3, 2018 •

edited

Loading

ChrisBr Dec 4, 2018

ChrisBr commented Dec 3, 2018

dmarcoux commented Dec 3, 2018 •

edited

Loading

bgeuken commented Dec 3, 2018

ChrisBr commented Dec 3, 2018

ChrisBr commented Dec 3, 2018 •

edited

Loading

ChrisBr commented Dec 3, 2018 •

edited

Loading

dmarcoux commented Dec 3, 2018

bgeuken left a comment

ChrisBr commented Dec 4, 2018

Ana06 left a comment

codecov bot commented Dec 4, 2018

codecov bot commented Dec 4, 2018

ChrisBr commented Dec 4, 2018

Bootstrap/monitor #6421

Bootstrap/monitor #6421

Conversation

ChrisBr commented Nov 30, 2018

obs-bot commented Nov 30, 2018

ChrisBr commented Nov 30, 2018

dmarcoux left a comment

Choose a reason for hiding this comment

Ana06 Dec 3, 2018

Choose a reason for hiding this comment

ChrisBr Dec 3, 2018

Choose a reason for hiding this comment

Ana06 Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

ChrisBr Dec 4, 2018

Choose a reason for hiding this comment

ChrisBr commented Dec 3, 2018

dmarcoux commented Dec 3, 2018 • edited Loading

bgeuken commented Dec 3, 2018

ChrisBr commented Dec 3, 2018

ChrisBr commented Dec 3, 2018 • edited Loading

ChrisBr commented Dec 3, 2018 • edited Loading

dmarcoux commented Dec 3, 2018

bgeuken left a comment

Choose a reason for hiding this comment

ChrisBr commented Dec 4, 2018

Ana06 left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2018

Codecov Report

codecov bot commented Dec 4, 2018

Codecov Report

ChrisBr commented Dec 4, 2018

Ana06 Dec 3, 2018 •

edited

Loading

dmarcoux commented Dec 3, 2018 •

edited

Loading

ChrisBr commented Dec 3, 2018 •

edited

Loading

ChrisBr commented Dec 3, 2018 •

edited

Loading