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

4372 - Notes filterable table component #4387

Merged
merged 6 commits into from
Nov 14, 2024

Conversation

WillNigel23
Copy link
Collaborator

No description provided.

@WillNigel23 WillNigel23 force-pushed the 4372-notes-filterable-table branch 2 times, most recently from ded977a to 9ad1ef6 Compare October 24, 2024 23:47

(function($) {

$.fn.filterableTable = function(options) {
Copy link
Collaborator Author

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?
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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)
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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')
Copy link
Collaborator Author

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 () {
Copy link
Collaborator Author

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('. ');
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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 @@
---
Copy link
Collaborator Author

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?

@WillNigel23 WillNigel23 force-pushed the 4372-notes-filterable-table branch from ed68f4f to 16ead55 Compare October 25, 2024 00:20
@WillNigel23 WillNigel23 requested a review from benwbrum October 25, 2024 00:20
@WillNigel23
Copy link
Collaborator Author

Refactored export/list here to reuse the filterable table component. Works well with different types of models without creating custom note_datatable class 😄

@WillNigel23
Copy link
Collaborator Author

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

@benwbrum benwbrum linked an issue Oct 28, 2024 that may be closed by this pull request
@benwbrum
Copy link
Owner

benwbrum commented Nov 5, 2024

I'm really pleased with this. The UI is good, and the performance is excellent. I spotted a few things that should be changed:

  • Let's get rid of the result number selector and use 50; 15 is too short for much of anything, and All has the potential to bring the site down for huge collections. _Update: I tested All on a huge collection and the site is taking forever to load, mainly due to rendering profile_picture.
  • Clicking on the contents of a note--A lot of figures need to be formatted in the screenshot below--sends the user to the show route for a collection, giving a NoSuchMethod exception. We should take the user to the page being displayed, anchored down to the note in the notes section.
  • Something's a little odd about the way we are wrapping the text for usernames. We should try to wrap on whitespace or word boundaries, or expand the width of the column if this is impossible. (See fransalman in the screenshot.)
  • We no longer have a link in the UI to this, but it would be nice to get notes/list for the whole site working again. Currently we get a NoSuchMethod exception if I try to visit http://localhost:3000/notes/list
  • I can start a search by tabbing away from the search field, but I cannot start a search by typing a term and hitting Enter

@WillNigel23
Copy link
Collaborator Author

WillNigel23 commented Nov 5, 2024

  • Clicking on the contents of a note--A lot of figures need to be formatted in the screenshot below--sends the user to the show route for a collection, giving a NoSuchMethod exception. We should take the user to the page being displayed, anchored down to the note in the notes section.

My bad for this, I forgot there is no show route for a note. I think we should retain the existing implementation where the Note column is showing plain text title instead of a link. I think there exists notes without collection(where the collection was deleted already I think, but the notes still exist). So we should instead just keep the text not as a link.

  • Something's a little odd about the way we are wrapping the text for usernames. We should try to wrap on whitespace or word boundaries, or expand the width of the column if this is impossible. (See fransalman in the screenshot.)

I see no screenshot. can you send it please? Not sure what needs to change in the appearance here

@WillNigel23
Copy link
Collaborator Author

Added anchor on link click for notes title

@benwbrum
Copy link
Owner

benwbrum commented Nov 7, 2024

This is all fantastic. My only remaining request is to change http://localhost:3000/notes?collection_id=i-demokratins-namn to a more restful route like /i-demokratins-namn/notes`

@benwbrum benwbrum merged commit d3dfb98 into development Nov 14, 2024
3 checks passed
@benwbrum benwbrum deleted the 4372-notes-filterable-table branch November 14, 2024 23:10
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.

Notes list (DataTable) fails to load with HTTP 500
2 participants