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

Using index mappers instead of own created storage for states of filtering #7319

Merged
merged 76 commits into from
Nov 23, 2020

Conversation

wszymanski
Copy link
Contributor

@wszymanski wszymanski commented Oct 7, 2020

Context

PR #7276 introduced a new kind of Index Map. I'm using it here for purpose of storing and managing states of filtering for particular columns. The Filters plugin has been adjusted to use the only Index Map (all unnecessary parts of code has been removed). What's more:

  • I removed clearConditions method of ConditionCollection element. There was a function that cleared conditions at the specified column index but without clearing stack order (stored in separate array). Since the order of columns is stored inside the Index Mapper there is no way to threaten it separately. Thus, some parts of the code have been changed. Hooks beforeClear and afterClear have been removed while afterRemove hook has been introduced.
  • I changed isMatchInConditions method of ConditionCollection element. It will need an argument. Function without an argument hasn't been used internally.
  • I introduced an extra method - named clearValue - for newly created Index Map within Using index mappers instead of own created storage for states of sorting #7276.
  • The internal "hiddenRowsCache" map was removed as it turned out that is not necessary at all. The UI components' hidden state can be calculated on the fly while showing the Dropdown
    menu.
  • The Filters UI components previously used the stateSaver mixing which causes the components state misalignment after column reordering. The mixin is removed and the state management from now on is implemented directly into the UI Base component.

How has this been tested?

Manual tests for alter actions.

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)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. Altering columns does not update filters attached to columns. #6830
  2. Sorting indicator moves incorrectly when adding a column #6397

Checklist:

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

wszymanski and others added 26 commits September 15, 2020 09:35
… unused method, changed type of indexes used in method documentation #6397
…putational complexity for some reads from the map - used mainly for rendering indicators #6397
Co-Authored-By: Wojciech Czerniak <wojciech.czerniak@gmail.com>
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 7, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f4cc152:

Sandbox Source
handsontable/examples: pull-request Configuration
handsontable/examples: pull-request Configuration
handsontable/examples: pull-request Configuration
handsontable/examples: pull-request Configuration

Additionally, the commit updates the TS definition.

Issue: #6830
The commit fixes a bug which occurs in column filtering state indication
within the columns' headers. The ConditionCollection class while closing
the dropdown menu unregistered the "filteringStates" map, where
shouldn't.
@budnix
Copy link
Member

budnix commented Nov 10, 2020

While testing the "by_value" component I've found another bug where the content of the component is not refreshed after columns reorder.
Kapture 2020-11-10 at 09 51 01

The commit removes the stateSaver mixin (which wasn't used broadly) and
directly implements the IndexMapper architecture into the Filters UI
components. Moreover, the changes remove the "hiddenRowsCache" map
which, as it turned out, is not necessary at all. The UI components'
hidden state can be calculated on the fly while showing the Dropdown
menu.

Issue: #6830
The code unnecessary call hide/show methods where while components'
state restoration the visibility state is already set.

Issue: #6830
Copy link
Contributor

@mrpiotr-dev mrpiotr-dev left a comment

Choose a reason for hiding this comment

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

Shouldn't we restore possible values after remove column with active filter_by_value?
altering_columns_filters

src/plugins/filters/conditionCollection.js Show resolved Hide resolved
@budnix
Copy link
Member

budnix commented Nov 20, 2020

Shouldn't we restore possible values after remove column with active filter_by_value?
altering_columns_filters

Filters are passive, so I'd say no. You can call the hot.filter() after column removal to achieve active-like behavior.

@wojciechczerniak wojciechczerniak dismissed budnix’s stale review November 22, 2020 14:25

@budnix took over this PR, should no longer be a reviewer

Copy link
Contributor

@wojciechczerniak wojciechczerniak left a comment

Choose a reason for hiding this comment

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

🥇

@budnix budnix merged commit d0700d8 into develop Nov 23, 2020
@budnix budnix deleted the feature/issue-6830 branch November 23, 2020 08:03
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.

None yet

4 participants