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 Datatables #863

Merged
merged 15 commits into from
Dec 23, 2021
Merged

Add Datatables #863

merged 15 commits into from
Dec 23, 2021

Conversation

lexiwitch
Copy link
Contributor

@lexiwitch lexiwitch commented Dec 20, 2021

Partial/WIP #821 & #821
Bonus: Fixes #806 - "Neighbourhood view not displaying names for parent objects"

Progress

Aster

  • Added 'ajax-datatables-rails' gem
  • Added 'datatables.net-bs4' yarn package
  • Added datatable css to application.css
  • Added datatable js to packs/application.js
  • Added datatable code to admin_index.js component
  • Changed neighbourhoods controller to allow datatables JSON response as well as HTML
  • Added datatables/neighbourhood_datatable.rb
  • Changed Neighbourhoods index so it renders it's own (data) table rather than admin_index component table. This was done to fix the "edit" column not really being supported by datatables

Kim

  • Converted datatable to a partial
  • Set up users, neighbourhoods and partners index page functionality
  • Created shared functionality where possible
  • Changed columns for neighbourhoods to something more usable
  • Redid neighbourhoods edit page to reflect changes to model
  • Updated tests to match new config
  • Quality of life: updated README and js versions
  • Created tickets for knock-on issues
  • Some refactoring

TODO

  • Get edit links working as per this documentation so we can actually edit the content
  • Add "parent" column to neighbourhoods table as a proof of concept of having relation columns, which currently seem to bug out the whole thing
  • Consider writing a ticket to migrate Mountain View to Draper, which seems more of a standard nowadays and what this gem seems to be asking for
  • Fix or make a new ticket to tidy up the edit neighbourhood pages
  • Bug: pressing "back" on browser loads two paginators
  • Update integration tests to work after js has loaded and not before as per Datatables docs
  • Add some integration testing for the paginator etc

Bonus: Fixes #806
  - "Neighbourhood view not displaying names for parent objects"

Added Datatables
- Added 'ajax-datatables-rails' gem
- Added 'datatables.net-bs4' yarn package
- Added datatable css to application.css
- Added datatable js to packs/application.js
- Added datatable code to admin_index.js component
- Changed neighbourhoods controller to allow datatables JSON response as
  well as HTML
- Added datatables/neighbourhood_datatable.rb
- Changed Neighbourhoods index so it renders it's own (data) table rather
  than admin_index component table. This was done to fix the "edit"
  column not really being supported by datatables
@katjam
Copy link
Member

katjam commented Dec 20, 2021

Closing test fail & need to make sure neighbourhood admins can see datatables

@katjam katjam closed this Dec 20, 2021
@kimadactyl kimadactyl reopened this Dec 21, 2021
@kimadactyl
Copy link
Member

kimadactyl commented Dec 21, 2021

This works as is, can either merge or do some tidyup first?

Key gotchas:

  • The app/components directory is deprecated for javascript, this just gets loaded the old fashioned way, we want stuff in the webpacker environment really
  • You need the special document.load syntax for turbolinks
  • Let's try and keep admin.js and application.js separate so as not to pollute the main app
  • I think the table ID for the datatables tables should be namespaced, i.e. js-datatable-neighbourhoods so there's no confusion where it comes from
  • [Big cleanup task] if poss I think we should put this back into the component structure now it's working - I don't see why it shouldn't! We prob also need to feed it a column_type and datatables_config object?

@kimadactyl kimadactyl self-assigned this Dec 21, 2021
@kimadactyl
Copy link
Member

I think we should change columns to reflect the database structure too as it's kinda confusing as is

@kimadactyl
Copy link
Member

Remove "Add new neighbourhood" button (and functionality?)

@kimadactyl kimadactyl linked an issue Dec 21, 2021 that may be closed by this pull request
12 tasks
@kimadactyl kimadactyl marked this pull request as draft December 21, 2021 18:57
@kimadactyl kimadactyl linked an issue Dec 22, 2021 that may be closed by this pull request
@kimadactyl
Copy link
Member

Everything done, new ticked added for out of scope stuff

@kimadactyl kimadactyl marked this pull request as ready for review December 22, 2021 14:45
@kimadactyl kimadactyl requested a review from katjam December 22, 2021 14:48
README.md Show resolved Hide resolved
Copy link
Member

@katjam katjam left a comment

Choose a reason for hiding this comment

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

I had a quick look. This looks great. Few tiny comments... but super excited to get this in. Is very well organised!

dataTable = $('#datatable').DataTable({
"processing": true,
"serverSide": true,
"pageLength": 15,
Copy link
Member

Choose a reason for hiding this comment

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

Will this be configurable?

Copy link
Member

@kimadactyl kimadactyl Dec 23, 2021

Choose a reason for hiding this comment

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

there's a little dropdown on the datatable to pick the number per page if that counts :) but yes it could be set per-page if we need? what do you mean by configurable?

app/datatables/neighbourhood_datatable.rb Show resolved Hide resolved
app/views/admin/neighbourhoods/_form.html.erb Outdated Show resolved Hide resolved
app/views/admin/neighbourhoods/edit.html.erb Outdated Show resolved Hide resolved
title: "Neighbourhoods",
model: :neighbourhoods,
columns: %i[id name unit_name unit_code_key unit_code_value],
data: @neighbourhoods.limit(15),
Copy link
Member

Choose a reason for hiding this comment

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

Again wondering if we want this to be configurable.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by configurable?

Copy link
Member

Choose a reason for hiding this comment

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

What you said on the other one... if there's a dropdown so the user can configure and this is just our default starting point, then all good. I could not tell if it was a fixed value.

</div>
</div>
</div>
<p class="alert alert-danger" role="alert">Warning: neighbourhoods should not be created here and this page is only left as a placeholder!</p>
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove it? What breaks if we do not have a placeholder?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm I did consider this but I think it's good to resolve the usual default Rails routes? The main usecase for it going forwards is for when we have custom geographies but that's a way off?

@kimadactyl kimadactyl merged commit 0bb508c into main Dec 23, 2021
@kimadactyl kimadactyl deleted the datatables_821 branch December 23, 2021 13:40
<h2>Geography</h2>
<div class="row">
<div class="col-md-6">
<h3>Is a part of&hellip;</h3>
Copy link
Member

Choose a reason for hiding this comment

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

😍

Copy link
Member

Choose a reason for hiding this comment

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

Great new headings!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants