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.

Sign up for GitHub

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

Jump to bottom

Change order of filters in the filters section #4095

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Change order of filters in the filters section #4095

wants to merge 13 commits into from

Conversation

sharath-1517
Copy link
Contributor

Fixes #4094

Technical details

  • I have ordered the filters according to the issue mentioned.
  • Added truncate component to the dropdown to avoid inconsistent text overflow.

Screenshots

image

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin 
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@sharath-1517
Copy link
Contributor Author

Hey @seancolsen I have changed the ui to be truncated as I mentioned in the issue conversation. But I have to yet figure out the filtering operations.

@seancolsen
Copy link
Contributor

@sharath-1517 Can you clarify this please?

But I have to yet figure out the filtering operations.

Is this PR ready for review? Or do you still have more work to do on it? I'm no sure what you mean by "filtering operations", given that the issue is clear about not changing any of the logic used to filter records.

@sharath-1517
Copy link
Contributor Author

Yes absolutely @seancolsen the PR is ready to review. I mentioned "But I have to yet figure out the filtering operations" because I thought that I should work on the filtering operations as well after seeing these points below,
github-1

Seems like that's not how it was and I have misunderstood the task. You can go on with the review for this PR. Thanks for your response and sorry for the inconvenience.

@seancolsen seancolsen self-assigned this Dec 19, 2024
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Dec 19, 2024
seancolsen
seancolsen requested changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

Thanks @sharath-1517

Your application of <Truncate> looks good.

Unfortunately there are still two problems here...

Labels need to be type-aware

When filtering a non-text column via "equals", the label for the comparison should read "equals" not "equals (case sensitive)". I would imagine this to be relatively straightforward to fix, though I don't know the exact steps. I'd like you to take a stab at addressing this issue.

User confusion will still arise

Hmmmm... This is where this change gets a little trickier than I initially thought. This PR kind of helps, but it doesn't fully address the original intent of the issue. The problem here was that users expected filtering to be done in a case insensitive manner but the "happy path" to applying a filter results in the filter being performed in a case-sensitive manner.

Here's why this PR doesn't fully solve the problem:

  1. Say I click "Filter" to open the filtering drop down.

  2. Then I click "Add New Filter".

    At this point, a filter condition will be added, with the first column chosen by default (say id). In most cases that first column will be a numeric column, so it won't have a "contains" comparison. So "equals" will be chosen as the comparison by default.

  3. Now, if I change id to some other text-like column, say name, then I'm right back to this same problem — we have case-sensitive filtering when we ought to have case-insensitive filtering.

What does work about this PR is that if I begin from the column header, open the context menu, and choose "Filter Column", then I get "contains" by default. That's great!

So here's what I think we should do...

When you open the "Group" dropdown, do you see how the "Add New Grouping" button opens a dropdown to select a column? I think we should implement that same UX for Filter. Let's also do it for Sort too, just to make them all consistent. This way, when you add a new filter condition, you'll begin with a specific column, not just the first column. I think this is actually an improvement anyway, because the user is unlikely to want to filter or sort by id.

Do you think you could try to figure out how to do that?

@seancolsen seancolsen assigned sharath-1517 and unassigned seancolsen Jan 3, 2025
@seancolsen seancolsen added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Jan 3, 2025
@sharath-1517
Copy link
Contributor Author

sharath-1517 commented Jan 4, 2025 •
edited
Loading

Hey @seancolsen thanks for getting back! Yeah sure I will try those two issues that you have mentioned above. I might get certain follow up questions on the go. I'll make sure I'll keep you posted as often as possible. Thanks again!

@seancolsen
Copy link
Contributor

@sharath-1517 heads up that we are aiming to have this issue resolved in the next couple weeks. If I don't see some significant progress here by the end of the week, I'll need to take it over myself. Sorry for the short notice on this. I should have made the time sensitivity more clear in the original issue.

@sharath-1517
Copy link
Contributor Author

@sharath-1517 heads up that we are aiming to have this issue resolved in the next couple weeks. If I don't see some significant progress here by the end of the week, I'll need to take it over myself. Sorry for the short notice on this. I should have made the time sensitivity more clear in the original issue.

Hey @seancolsen I will surely get you an update before this weekend. Thanks for letting me know about this!

@sharath-1517
Copy link
Contributor Author

Hey @seancolsen I have tried the grouping like dropdown in filtering. I used Grouping components code and it's dependencies as base and tried changing like the same for filters. I want to make sure I am going in the right track. Could you take a look at it please?

PR Ready for review

@seancolsen seancolsen assigned seancolsen and unassigned sharath-1517 Jan 7, 2025
@seancolsen seancolsen added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Jan 7, 2025
@sharath-1517
Copy link
Contributor Author

sharath-1517 commented Jan 10, 2025 •
edited
Loading

PR Ready for a review

Edit: You can ignore this PR Review, since this issue needs more time to get it straight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seancolsen seancolsen seancolsen requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@seancolsen seancolsen

Labels
pr-status: review A PR awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change order and labels of filtering comparisons
2 participants
@sharath-1517 @seancolsen