-
Notifications
You must be signed in to change notification settings - Fork 51
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
4372 - Notes filterable table component #4387
base: development
Are you sure you want to change the base?
Conversation
ded977a
to
9ad1ef6
Compare
|
||
(function($) { | ||
|
||
$.fn.filterableTable = function(options) { |
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.
Let me explain the approach here.
Right now I settled with least amount of architecture to make this work. Bulk of the work is going to be done server/rails side instead of here in js. In any case, for further customizations we can pass in options
when initializing, but we can deal with that once we encounter the need for adding further customization.
So the approach is, for filterable tables, we have a form and the actual table is inside a form under a specific selector id of our choosing (ex: "#notes-table").
Each filterable table will have by default, hidden fields for :sort
and :order
, params that we will use later in filtering/sorting in server side.
Then the simplest approach I could think of here is, we autosubmit when the form changes, make the xhr request, then server will give us an html partial as response and we jquery substitute the contents of '#table-selector' with the response of the server.
So the structure is
#form
hidden_fields :sort, :order
form_fields :per_page, :search, :etc
#table // this is what we replace with server response partial
For sorting, we just attach event listeners for each sortable table-header, which is identifiable by 'data-sort' attr.
def index | ||
filtered_notes | ||
|
||
render partial: 'table' if request.xhr? |
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.
Ideally we use turbo_streams here, but that will have to wait until upgrade.
But this is similar pattern, as if we are doing format.html, and format.json, but in this case, if request.xhr we will render just the partial that we will use to replace the table contents.
@note = Note.find(params[:id]) | ||
private | ||
|
||
def filtered_notes |
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.
For filtering/sorting actions, as much as possible we should do it server-side and as efficient as possible (avoiding n+1 queries if possible)
I think this is the simplest I can think of in order to have the notes table searchable by all columns. But if possible I'd like for us to just limit the search to note titles maybe? But if we really need to search with all available columns then this should work.
@@ -0,0 +1,43 @@ | |||
module Components::FiltersHelper | |||
|
|||
def fe_filter_table_wrapper(url:, selector:, sorting:, ordering:, static_params: {}, &block) |
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 is the functional component approach I am taling about. Much like rendering a shared/partial but we hand it off first inside a ruby function so that we can do complex operations/params processing BEFORE handing it off to the partial. This way we avoid bulky ruby logic inside .html.slim files while still retaining the ability to re-use components easily.
fe_
prefix for these functions just to denote that these are frontend
component functions.
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.
Moreover here, I personally prefer named parameters so that we technically require the necessary variables, much like setting locals in partials. Though in terms of locals, we can easily miss when local_assigns was not set correctly I think.
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.
We can add params **opts though, if we really need more customizations in the future if we have optional parameters that does not make sense adding as explicit function params, but for now this architecture should serve what our datatables need.
table.datagrid.striped.dataTable.no-footer | ||
thead | ||
=fe_sortable_header(key: :title, sorting: @sorting, ordering: @ordering, classes: 'w80') do | ||
=t('export.index.work_title') |
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.
Using functional components, we simplified html.slim here from
thead
- sorting_class = "sorting_#{params[:order] || 'asc'}"
th[class="w80 sorting #{params[:sort] == 'title' ? sorting_class : '' }" data-sort='title']
=t('export.index.work_title')
th[class="sorting #{params[:sort] == 'page_count' ? sorting_class : '' }" data-sort='page_count']
=t('export.index.pages')
-unless collection.subjects_disabled
th[class="sorting #{params[:sort] == 'indexed_count' ? sorting_class : '' }" data-sort='indexed_count']
=t('export.index.indexed')
th[class="sorting #{params[:sort] == 'completed_count' ? sorting_class : '' }" data-sort='completed_count']
=header
th[class="sorting #{params[:sort] == 'reviewed_count' ? sorting_class : '' }" data-sort='reviewed_count']
=t('export.index.review')
th.w100 =t('export.index.progress')
th(colspan="6") =t('export.index.export_as')
to
=fe_sortable_header(key: :title, sorting: @sorting, ordering: @ordering, classes: 'w80') do
=t('export.index.work_title')
=fe_sortable_header(key: :page_count, sorting: @sorting, ordering: @ordering) do
=t('export.index.pages')
-unless @collection.subjects_disabled
=fe_sortable_header(key: :indexed_count, sorting: @sorting, ordering: @ordering) do
=t('export.index.indexed')
=fe_sortable_header(key: :completed_count, sorting: @sorting, ordering: @ordering) do
=@header
=fe_sortable_header(key: :reviewed_count, sorting: @sorting, ordering: @ordering) do
=t('export.index.review')
th.w100 =t('export.index.progress')
th(colspan="6") =t('export.index.export_as')
We add in the benefit of making sure these components will produce similar looks as we are using partials, while maintaining the ability to add customizations like adding 'w80' class to some table headers here that need it.
@@ -58,68 +54,3 @@ p | |||
}, 1000); | |||
}); | |||
}); | |||
|
|||
$(document).ready( function () { |
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.
No need for custom js per data table now.
} else { | ||
// Error occurred | ||
var message = xhr.responseJSON.join('. '); | ||
var message = xhr.responseJSON.errors.join('. '); |
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.
I find that adding/editing notes does not show the flash messages. I added these and modified the json response so all responses in notes_controller now are technically json, but we can still get the flash messages.
@@ -0,0 +1,50 @@ | |||
table.datagrid.striped.dataTable.no-footer |
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 is the pattern for the tables I think we can follow for filterable tables. Do we need a usage docs for this @benwbrum ?
@@ -0,0 +1,7 @@ | |||
--- |
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.
Do the translations look okay to you?
ed68f4f
to
16ead55
Compare
Refactored export/list here to reuse the filterable table component. Works well with different types of models without creating custom note_datatable class 😄 |
We still have some tables using old DataTables. I can refactor them on other tickets but for now focused on notes as this ticket is already large |
No description provided.