-
Notifications
You must be signed in to change notification settings - Fork 985
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
fixes #10985 - Set css class for width of filter select on puppetca#index #2511
Conversation
There were the following issues with the commit message:
Guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
per @ohadlevy idea, moved those fields to use data-tables |
@@ -4,7 +4,7 @@ def index | |||
# expire cache if forced | |||
Rails.cache.delete("ca_#{@proxy.id}") if params[:expire_cache] == "true" | |||
certs = find_certs | |||
@certificates = certs.sort.paginate :page => params[:page], :per_page => Setting::General.entries_per_page | |||
@certificates = certs.sort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should apply client side sorting instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous behavior was that they arrive sorted (sort from the server) and the can be re-sorted on the client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i understand, imho you should set the default sort on the js side now, as it has no knowledge on which field sorting was done
@@ -77,7 +77,8 @@ function onContentLoad(){ | |||
$('[data-table=inline]').not('.dataTable').dataTable( | |||
{ | |||
"sDom": "<'row'<'col-md-6'f>r>t<'row'<'col-md-6'i><'col-md-6'p>>", | |||
"sPaginationType": "bootstrap" | |||
"sPaginationType": "bootstrap", | |||
"order": [[ 0, 'asc' ]] // Set default order for the first column |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will sort each data-table with the first column (usually name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left this on this PR even though not using data-tables (as it is a good practice to have default sort)
@@ -3,8 +3,7 @@ def index | |||
@proxy = find_proxy | |||
# expire cache if forced | |||
Rails.cache.delete("ca_#{@proxy.id}") if params[:expire_cache] == "true" | |||
certs = find_certs | |||
@certificates = certs.sort.paginate :page => params[:page], :per_page => Setting::General.entries_per_page | |||
@certificates = find_certs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method returns only valid and pending certificates by default, then it used to be possible to select "revoked" from the dropdown (in theory) to view revoked certs too. I can't see how to do this now? Also, the find_certs method still uses params[:state]
which is no longer set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed find_certs
method and now returning all certs. Filtering is done only on the client side. Seems reasonable?
@shlomizadok imho the usability here is worse, its hard to filter, and at first glance you lose visibility. by default imho, the user is looking for pending certs (else why go there), sorting based on name is useless if you have uuid for certnames. i wonder if there is a way to keep the old drop down, and apply it via search to data-table or something that will keep the old ux and improve the client side sorting. |
So why the suggestion that it should move to the JS table impl if we lose the filtering feature in the process? We're moving further and further from fixing the original bug it appears. |
Reverted back the changes to autosign controller & view. |
@@ -77,7 +77,8 @@ function onContentLoad(){ | |||
$('[data-table=inline]').not('.dataTable').dataTable( | |||
{ | |||
"sDom": "<'row'<'col-md-6'f>r>t<'row'<'col-md-6'i><'col-md-6'p>>", | |||
"sPaginationType": "bootstrap" | |||
"sPaginationType": "bootstrap", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused?
I think it's probably due to mark_translated then which makes the text a little longer. Is the width of that text container limited precisely to the length of the English text? If so then it's going to wrap in any language where "Filter by state" translates to a slightly longer string. |
the text container is not limited, the Looks reasonable? (though, to be honest, I don't understand why we give so little space on https://github.com/theforeman/foreman/blob/develop/app/views/layouts/application.html.erb#L13 [ |
re[test] - RKF |
[test] |
Merged as befd671, thanks @shlomizadok. |
Please note that at some point we will refactor this page to work with javascript filtering