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

ESF on igx-grid does not reflect current filters in esf dialog #14328

Merged
merged 52 commits into from
Jul 2, 2024

Conversation

ddincheva
Copy link
Contributor

Closes #14188

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@hanastasov hanastasov added the ❌ status: awaiting-test PRs awaiting manual verification label Jun 10, 2024
@hanastasov hanastasov marked this pull request as ready for review June 11, 2024 06:11
@hanastasov hanastasov added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Jun 13, 2024
@deyvidnenchev
Copy link
Contributor

b3.mp4

@deyvidnenchev
Copy link
Contributor

deyvidnenchev commented Jun 18, 2024

According to this comment we must show the number of column filtered not the number of filtering operands (in advanced filtering we can apply multiple operands to one column)
ec1

In this screenshot since we have two operands on column ID the number on the Advanced filtering button should be 1

@AnjiManova
Copy link

AnjiManova commented Jun 28, 2024

Figma Design File and references related to the issue

We suggest removing the custom button border style after adding a span with a number of the filtered columns. There is no need to add additional visual indications or customize the buttons this way.

image image

Please apply and point 1 from the last screenshot -> when filters are applied. The Text filter list item should be in an Active state, the same as the List component.

@AnjiManova
Copy link

AnjiManova commented Jun 28, 2024

Create a new issue from this comment because it is not related to this PR

I noticed something else, and we talked with Desi: This is an additional issue that should be logged, but we can create such from this comment here.

When I select one list item, the Text filter is changed to Text filter (1), and "Equal" is selected from the filters.
When I select two list items, the Text filter is changed to Text filter (2), and the "Custom filter" is selected from the filters. However, when I select three items, the Text filter is changed to Text filter (1), and there is no selection in the Text filters. Shouldn't it be showing the "Custom filter" again? This is replicated from the Master, and it is an issue there as well.

Screen.Recording.2024-06-28.at.11.12.37.mov

I believe the logic should be: when two or more items are selected to apply the Custom filter, the filters should be indicated with (n) next to the Text filter.

Something interesting here is that everything looks fine if I select two items and then add more from the Custom filter. After manually adding an additional filter, there is no indication of which items/filters are selected.

Screen.Recording.2024-06-28.at.11.12.58.mov

@hanastasov hanastasov self-requested a review June 28, 2024 12:09
@hanastasov
Copy link
Contributor

Please add one more test, similar to Should show the previously entered filter value when reopen esf dialog, but one that actually selects a different condition from the dropdown, thus should reopen the dialog with empty conditions. This is one of my the main scenarios that we determined as being buggy from very beginning, so it should have a test

@hanastasov
Copy link
Contributor

hanastasov commented Jun 28, 2024

Create a new issue from this comment because it is not related to this PR

I noticed something else, and we talked with Desi: This is an additional issue that should be logged, but we can create such from this comment here.

When I select one list item, the Text filter is changed to Text filter (1), and "Equal" is selected from the filters. When I select two list items, the Text filter is changed to Text filter (2), and the "Custom filter" is selected from the filters. However, when I select three items, the Text filter is changed to Text filter (1), and there is no selection in the Text filters. Shouldn't it be showing the "Custom filter" again? This is replicated from the Master, and it is an issue there as well.

Screen.Recording.2024-06-28.at.11.12.37.mov
I believe the logic should be: when two or more items are selected to apply the Custom filter, the filters should be indicated with (n) next to the Text filter.

Something interesting here is that everything looks fine if I select two items and then add more from the Custom filter. After manually adding an additional filter, there is no indication of which items/filters are selected.

Screen.Recording.2024-06-28.at.11.12.58.mov

Nice catch, @AnjiManova !

This is due to the fact how ESF (Excel Style Filtering) works:

  • For relatively small number of items selected through the checkboxes - an "equals" filtering condition is created for each item. So, for 2 items selected - two filtering conditions are created, therefor you see Text filter (2) displayed.
  • When the number of items pass a certain threshold - like when you selected the third item, a single condition of different type is created (IIRC the condition is called "in" - like the filtered records should be "in" the collection of selected items, smth like that) - therefore now you see Text filter (1)

I feel this is kind of an edge case and not even sure if a bug, so yes - please log it as a separate issue, tagged with question label

Copy link
Contributor

@hanastasov hanastasov left a comment

Choose a reason for hiding this comment

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

Since the comment about extracting the code to initialize expression in a new method was resolved without the comment being addressed, I am giving an example what I imagined for a method without this repetitive code.:

    private createInitialExpressionUIElement() {
        const firstExprUI =  this.initializeExpressionUI();
        const secondExprUI = this.initializeExpressionUI();
 
        if (this.expressionsList.length === 1 && this.expressionsList[0]?.expression?.condition?.name === this.selectedOperator) {
            ...
        } else {
            ...
        }

        firstExprUI.afterOperator = FilteringLogic.And;
        secondExprUI.beforeOperator = FilteringLogic.And;
        this.expressionsList = [firstExprUI ,secondExprUI];
    }

The other changes in the method are a custom preference of course, but it makes it look more readable.

@hanastasov hanastasov merged commit e36f580 into master Jul 2, 2024
5 of 6 checks passed
@hanastasov hanastasov deleted the ddincheva/filteringDialog branch July 2, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data grid filtering:Programatic filtering on data-grid does not reflect selection in filter dialog
5 participants