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

Change order of filters in the filters section #4095

Open
wants to merge 4 commits into
base: develop
Choose a base branch
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: revision A PR awaiting follow-up work from its author after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change order and labels of filtering comparisons
2 participants