Expose DataTables customisation options #25
Conversation
rob-duplock
commented
Jul 3, 2018
•
rob-duplock
commented
- Added page length and searching options.
- Refactored couple comments.
- Drupal coding standard updates.
- Code review changes
- Refactored couple comments. - Drupal coding standard updates (DH)
There was a problem hiding this comment.
Sorry @rob-duplock I have left quite a few comments for you to address. I am being super strict because we area aiming for a release soon and I don't want anything to go in that we will need to change later.
js/dvfTables.js
Outdated
| @@ -9,7 +9,7 @@ | |||
| attach: function (context) { | |||
| $('[data-dvftables]', context).each(function () { | |||
| var $table = $(this), | |||
| tableId = $table.data('dvftables'); | |||
| tableId = $table.data('dvftables'); | |||
There was a problem hiding this comment.
Can we undo this change please?
| '#type' => 'select', | ||
| '#title' => $this->t('Page length'), | ||
| '#options' => $this->getPageLengthOptions(), | ||
| '#description' => $this->t('Number of rows to display on a single page when using pagination.'), |
There was a problem hiding this comment.
Pagination is always enabled. Can we please change this to:
Number of rows per page.
| '#default_value' => $this->config('table', 'table_options', 'page_length'), | ||
| ]; | ||
|
|
||
| $form['table']['table_options']['enable_searching'] = [ |
There was a problem hiding this comment.
For consistency with the other settings, can this please be renamed to searching?
|
|
||
| $form['table']['table_options']['enable_searching'] = [ | ||
| '#type' => 'checkbox', | ||
| '#title' => $this->t('Enable Searching'), |
There was a problem hiding this comment.
For consistency again, Searching should be lowercase.
| @@ -94,4 +113,21 @@ protected function rowHeaderField() { | |||
| return $this->config('table', 'row_header_field'); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns Page length options. | |||
There was a problem hiding this comment.
Returns page length options.
| @@ -22,6 +22,10 @@ public function defaultConfiguration() { | |||
| 'table' => [ | |||
| 'table_header_field' => '', | |||
| 'row_header_field' => '', | |||
| 'table_options' => [ | |||
js/jquery.dvfTables.js
Outdated
| @@ -28,6 +28,7 @@ | |||
| this | |||
| .parseData() | |||
| .parseColumns() | |||
| .getTableOptions() | |||
There was a problem hiding this comment.
For consistency, this should be .parseTableOptions.
js/jquery.dvfTables.js
Outdated
| return this; | ||
| }, | ||
|
|
||
| getTableOptions: function () { |
There was a problem hiding this comment.
Can we please rename this to parseTableOptions?
js/jquery.dvfTables.js
Outdated
| }, | ||
|
|
||
| getTableOptions: function () { | ||
| this.config = $.extend(this.config, this.options.tableOptions); |
There was a problem hiding this comment.
After all my suggestions this.options.tableOptions may not be set so we need to check before using it.
parseTableOptions: function () {
if (this.options.tableOptions) {
this.config = $.extend(this.config, this.options.tableOptions);
}
return this;
}
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected function getTableOptions() { |
There was a problem hiding this comment.
This can be removed. Please see my comments below.
|
Thanks @jorgegc please see latest commit. |
There was a problem hiding this comment.
Solid work @rob-duplock. Sorry about the annoying comments!
