Rewrite plugin Search to es6. #4892

pnowak · 2018-03-02T13:58:35Z

Context

Search is now based on BasePlugin.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature or improvement (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issue(s):

Rewrite plugin Search on es6. #4629

Checklist:

My code follows the code style of this project,
My change requires a change to the documentation.

…helpers and afterInit hook instead of beforeRenderer #4629

…ion isUndefined, cost #4629

…re readable tests #4629

wszymanski · 2018-03-27T13:31:04Z

src/plugins/search/search.js

+  disablePlugin() {
+    this.hot.addHook('beforeRenderer', (TD, row, col, prop, value, cellProperties) => this.onBeforeRenderer(TD, row, col, prop, value, cellProperties));
+    this.hot.addHookOnce('afterRender', () => {
+      this.hot.removeHook('beforeRender', (TD, row, col, prop, value, cellProperties) => this.onBeforeRenderer(TD, row, col, prop, value, cellProperties));


There should be a reference to the proper function.

wszymanski · 2018-03-27T13:32:25Z

src/plugins/search/search.js

-function init() {
-  var instance = this;
+    if (this.isEnabled() && cellProperties.isSearchResult) {
+      if (!cellProperties.className.includes(this.searchResultClass)) {


It could be done within util. Property className can be also an array.

wszymanski · 2018-03-27T13:39:12Z

src/plugins/search/search.js

+    const searchSettings = this.hot.getSettings().search;
+    this.checkPluginSettings(searchSettings);
+
+    this.addHook('beforeRenderer', (TD, row, col, prop, value, cellProperties) => this.onBeforeRenderer(TD, row, col, prop, value, cellProperties));


Maybe rest parameters would look more clear? Please apply that also to rest of places where hooks are added.

wszymanski · 2018-03-28T08:57:17Z

src/plugins/search/test/search.e2e.js

-        }
-      }
+      let cell = hot.getCell(0, 0);
+      expect($(cell).hasClass(hot.getPlugin('search').searchResultClass)).toBe(false);


Please simplify this calls here and in next tests.

wszymanski · 2018-03-28T08:57:34Z

src/plugins/search/test/search.e2e.js

-      for (var rowIndex = 0, rowCount = countRows(); rowIndex < rowCount; rowIndex++) {
-        for (var colIndex = 0, colCount = countCols(); colIndex < colCount; colIndex++) {
-          var callArgs = searchCallback.calls.argsFor((rowIndex * 5) + colIndex);
+      for (let rowIndex = 0, rowCount = countRows(); rowIndex < rowCount; rowIndex += 1) {


Please test it without looping.

wszymanski · 2018-03-28T08:58:20Z

src/plugins/search/search.js

-function SearchCellDecorator(instance, TD, row, col, prop, value, cellProperties) {
-  var searchResultClass = (cellProperties.search !== null && typeof cellProperties.search == 'object' &&
-      cellProperties.search.searchResultClass) || Search.global.getDefaultSearchResultClass();
+    if (cellProperties.className) {


Please add tests. Please cache the cellProperties.className.

wszymanski · 2018-03-28T08:59:48Z

src/plugins/search/search.js

+  disablePlugin() {
+    const beforeRendererCallback = (TD, row, col, prop, value, cellProperties) => this.onBeforeRenderer(TD, row, col, prop, value, cellProperties);
+
+    this.hot.addHook('beforeRenderer', beforeRendererCallback);


Please add tests.

wszymanski · 2018-03-28T09:02:31Z

src/plugins/search/search.js

+     */
+    this.queryMethod = DEFAULT_QUERY_METHOD;
+    /**
+     *  Class added to each cell that belongs to the search.


Good direction 👍 Maybe change it yet to: Class added to each cell that belongs to the searched query.

wszymanski · 2018-03-28T09:03:42Z

src/plugins/search/search.js

+  constructor(hotInstance) {
+    super(hotInstance);
+    /**
+     * Callback function is responsible for marking the cells belonging to the search by setting property to it.


Please describe the property, not the value.

wszymanski · 2018-03-28T09:10:56Z

src/plugins/search/search.js

+   * Query method - used inside search input listener.
+   *
+   * @param {String} queryStr Searched value.
+   * @param {Function} [callback=DEFAULT_CALLBACK] Callback function responsible for setting the cell's `isSearchResult` property.


Please change from [callback=DEFAULT_CALLBACK] to [callback]. Apply the same rule below.

* Rewrite the search plugin to ES6 handsontable#4629 * Problem: default that were not default. Solution: method rename, use helpers and afterInit hook instead of beforeRenderer handsontable#4629 * After CR - rename searchResultClass to elementClass, use helper function isUndefined, cost handsontable#4629 * Rest parametrs replace arguments handsontable#4629 * After CR - Default parameter and more descritions handsontable#4629 * Fix demo page handsontable#4629 * const and cell(td) not element handsontable#4629 * searchResultClass come back handsontable#4629 * searchResultClass come back in test too handsontable#4629 * improvement of comments handsontable#4629 * After CR: new method, settings descriptive, improve test handsontable#4629 * beforeRenederer hook replace extension base renderer handsontable#4629 * Improve hooks after disablePlugin plus write test handsontable#4629 * After CR: beforeRenderer instance, className can also be Array and more readable tests handsontable#4629 * After CR - more test and tiny fixes handsontable#4629 * Changed: variables descriptions. Added: test for onBeforeRenderer. handsontable#4629 * Changed: simple comparision instead of loops. handsontable#4629 * onBeforeRenderer adjustments. handsontable#4629

pnowak added 11 commits December 8, 2017 16:30

Rewrite the search plugin to ES6 #4629

9c29aa0

Problem: default that were not default. Solution: method rename, use …

d0fc528

…helpers and afterInit hook instead of beforeRenderer #4629

After CR - rename searchResultClass to elementClass, use helper funct…

877c569

…ion isUndefined, cost #4629

Rest parametrs replace arguments #4629

b3efeb6

After CR - Default parameter and more descritions #4629

553cb14

Fix demo page #4629

1ca092d

const and cell(td) not element #4629

bb6ecb6

searchResultClass come back #4629

a8eda56

searchResultClass come back in test too #4629

71b58ba

improvement of comments #4629

ff07d93

After CR: new method, settings descriptive, improve test #4629

e27c761

pnowak requested a review from wszymanski March 2, 2018 13:59

merge with develop #4629

f7f3468

wszymanski removed their request for review March 6, 2018 09:49

pnowak requested a review from wszymanski March 23, 2018 10:58

beforeRenederer hook replace extension base renderer #4629

bf045c9

pnowak assigned wszymanski Mar 26, 2018

Improve hooks after disablePlugin plus write test #4629

a440e01

wszymanski assigned pnowak and unassigned wszymanski Mar 27, 2018

After CR: beforeRenderer instance, className can also be Array and mo…

7688cc3

…re readable tests #4629

pnowak assigned wszymanski and unassigned pnowak Mar 28, 2018

wszymanski suggested changes Mar 28, 2018

View reviewed changes

wszymanski reviewed Mar 28, 2018

View reviewed changes

wszymanski suggested changes Mar 28, 2018

View reviewed changes

wszymanski assigned pnowak and unassigned wszymanski Mar 28, 2018

After CR - more test and tiny fixes #4629

f71d01d

mrpiotr-dev assigned mrpiotr-dev and unassigned pnowak Apr 3, 2018

mrpiotr-dev added this to the April 2018 milestone Apr 3, 2018

Changed: variables descriptions. Added: test for onBeforeRenderer. #4629

664c47f

mrpiotr-dev assigned wszymanski and unassigned mrpiotr-dev Apr 3, 2018

wszymanski assigned mrpiotr-dev and unassigned wszymanski Apr 4, 2018

mrpiotr-dev added 2 commits April 4, 2018 15:33

Changed: simple comparision instead of loops. #4629

fe051dc

onBeforeRenderer adjustments. #4629

b0c8d92

wszymanski assigned wszymanski and pnowak and unassigned mrpiotr-dev and wszymanski Apr 4, 2018

wszymanski approved these changes Apr 4, 2018

View reviewed changes

wszymanski merged commit 964c091 into develop Apr 4, 2018

AMBudnik added the Status: Released label Apr 11, 2018

wojciechczerniak deleted the feature/issue-4629 branch October 17, 2018 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite plugin Search to es6. #4892

Rewrite plugin Search to es6. #4892

pnowak commented Mar 2, 2018 •

edited by jansiegel

wszymanski Mar 27, 2018

wszymanski Mar 27, 2018

wszymanski Mar 27, 2018

pnowak Mar 28, 2018

wszymanski Mar 28, 2018

pnowak Mar 28, 2018

wszymanski Mar 28, 2018

wszymanski Mar 28, 2018

pnowak Mar 28, 2018

wszymanski Mar 28, 2018

wszymanski Mar 28, 2018 •

edited

pnowak Mar 28, 2018

wszymanski Mar 28, 2018

wszymanski Mar 28, 2018

pnowak Mar 28, 2018

Rewrite plugin Search to es6. #4892

Rewrite plugin Search to es6. #4892

Conversation

pnowak commented Mar 2, 2018 • edited by jansiegel

Context

Types of changes

Related issue(s):

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wszymanski Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowak commented Mar 2, 2018 •

edited by jansiegel

wszymanski Mar 28, 2018 •

edited