-
Notifications
You must be signed in to change notification settings - Fork 9
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
Excel like behavior for grid column filtering #3894
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otito and I went over this several times and ironed out the kinks. My current comments are just minor linting nits.
We confirmed that the new implementation works like Excel.
We tested the following flows/cases:
- commitOnChange: true|false
- regular grid & tree grid
- statefulness when switching from value to custom tab
- hide/show "add to existing filter"
- grid font size tiny, standard, large, etc.
There is a lot to test, I don't think we missed anything, but anyone reviewing should really try to break it.
desktop/cmp/grid/impl/filter/headerfilter/values/ValuesTabModel.ts
Outdated
Show resolved
Hide resolved
Great thanks guys - I would like to have a look at this. Will do so today and reach out with any questions. |
Have only had a quick chance to try this out - will look more closely, and compare with Excel - but I ran into a usability issue pretty quick where I wanted to search for and select multiple values that required different searches to find. E.g. using Toolbox's primary grid filtering example, let's say:
Seems problematic, no? Will be interested to see how the Excel model we are looking to track handles this same "search for and filter this then that" use case. |
Well, I see Excel (on my Mac) doing something very similar - to be honest, it feels just as confusing if not more in Excel, not sure how to feel about it but a good thing in terms of our goal of replicating Excel. It's surprising how difficult I find it to intuit how this control works (or how I want it to work, speaking about the Excel version just as much as ours). But wanted to chat back here quick to confirm what you say above re. this working much like Excel |
OK - last comment for now - very helpful to try on Windows, as that's clearly (and correctly!) the version that matches the behavior we're implementing with this change. Note that Excel for Windows does not show the "auto apply" checkbox we see on Mac above. I was wondering what the default Line 93 in 16d74c3
I am thinking it should be |
The default commit on change is True, should I make the change to have it false and update the documentation?
|
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
develop
branch as of last change.breaking-change
label + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.