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

Rewrite plugin Search to es6. #4892

Merged
merged 19 commits into from Apr 4, 2018

Conversation

@pnowak
Copy link
Member

commented Mar 2, 2018

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):

  1. #4629

Checklist:

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

@pnowak pnowak requested a review from wszymanski Mar 2, 2018

@wszymanski wszymanski removed their request for review Mar 6, 2018

@pnowak pnowak requested a review from wszymanski Mar 23, 2018

@wszymanski wszymanski assigned pnowak and unassigned wszymanski Mar 27, 2018

@pnowak pnowak assigned wszymanski and unassigned pnowak Mar 28, 2018

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));

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

There should be a reference to the proper function.

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

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

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

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));

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

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

This comment has been minimized.

Copy link
@pnowak

pnowak Mar 28, 2018

Author Member

Done.

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

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

Please simplify this calls here and in next tests.

This comment has been minimized.

Copy link
@pnowak

pnowak Mar 28, 2018

Author Member

Done.

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) {

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

Please test it without looping.

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) {

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

Please add tests. Please cache the cellProperties.className.

This comment has been minimized.

Copy link
@pnowak

pnowak Mar 28, 2018

Author Member

Done.

disablePlugin() {
const beforeRendererCallback = (TD, row, col, prop, value, cellProperties) => this.onBeforeRenderer(TD, row, col, prop, value, cellProperties);

this.hot.addHook('beforeRenderer', beforeRendererCallback);

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

Please add tests.

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

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

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

This comment has been minimized.

Copy link
@pnowak

pnowak Mar 28, 2018

Author Member

Done.

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

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

Please describe the property, not the value.

* 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.

This comment has been minimized.

Copy link
@wszymanski

wszymanski Mar 28, 2018

Member

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

This comment has been minimized.

Copy link
@pnowak

pnowak Mar 28, 2018

Author Member

Done.

@wszymanski wszymanski assigned pnowak and unassigned wszymanski Mar 28, 2018

@swistach swistach assigned swistach and unassigned pnowak Apr 3, 2018

@swistach swistach added this to the April 2018 milestone Apr 3, 2018

@swistach swistach assigned wszymanski and unassigned swistach Apr 3, 2018

@wszymanski wszymanski assigned swistach and unassigned wszymanski Apr 4, 2018

@wszymanski wszymanski assigned wszymanski and pnowak and unassigned swistach and wszymanski Apr 4, 2018

@wszymanski wszymanski merged commit 964c091 into develop Apr 4, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No dependency changes
Details
security/snyk - package.json No dependency changes
Details
Mohammed90 added a commit to Mohammed90/handsontable that referenced this pull request Apr 28, 2018
Rewrite plugin Search to es6. (handsontable#4892)
* 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

@wojciechczerniak wojciechczerniak deleted the feature/issue-4629 branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.