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

feat: refactor filters on insights response table #17796

Merged

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Nov 22, 2024

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-4811 (Linear issue number - should be visible at the bottom of the GitHub issue description)
Screenshot.2024-12-02.at.11.55.25.mp4

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

Explanation

(Had to split due to the file size limit)

Part 1.

Screenshot.2024-11-28.at.14.29.48-trimmed.mp4

Part 2.

Screenshot.2024-11-28.at.14.29.48-trimmed.2-converted.mp4

Copy link
Contributor

@Udit-takkar Udit-takkar left a comment

Choose a reason for hiding this comment

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

@eunjae-lee There are a lot of unrelated changes in yarn.lock file. I don't think we should update all the dependencies in this PR

@eunjae-lee
Copy link
Contributor Author

@eunjae-lee There are a lot of unrelated changes in yarn.lock file. I don't think we should update all the dependencies in this PR

Hey @Udit-takkar, I made a change to packages/prisma/package.json but yes, I think my yarn.lock change might be wrong, because it's always changed even if I didn't touch package.json. Will it be solved if I configure submodules on my mac?

Copy link
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

Nice work! Still got some changes that i think are blocking before merging as we need to make this perfect i think.

loom: https://www.loom.com/share/df966a20c2d7453dbf475b6ce78495f0

But TLDR:

  • I can't seem to get contains/is/etc or any text filters to actually work
  • We've lost the "clear filters" button
  • People shows as a default even when i havent selected the filter
  • The "Add Filter" button seems to be on the wrong side of "All" Feels a bit weird when they appear on the right of the "all" button

Gimme a shout if you have any questions or would like to pair on getting these resolved

@eunjae-lee eunjae-lee force-pushed the eunjae/cal-4760-insights-text-filter-on-routing-list branch from 5442be7 to ecec66b Compare December 4, 2024 13:55
@eunjae-lee
Copy link
Contributor Author

@sean-brydon As we discussed separately,

  1. Fixed the text filter
  2. Added "Clear filters" button back (but I re-created to clear both filters from FilterContext and DataTable filters state)

Note for others:
It's not ideal to have all the filters from FilterContext scattered on the header area, but it takes a lot of time to migrate the filters from FilterContext to DataTable's filters state (query params activeFilters=...), so we keep the both system for now.

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@formkit/[email protected] None 0 35.4 kB justin-schroeder
npm/[email protected] None 0 221 kB dcode
npm/[email protected] None 0 669 kB kewisch

🚮 Removed packages: npm/@bcoe/[email protected], npm/@humanwhocodes/[email protected], npm/@isaacs/[email protected], npm/@istanbuljs/[email protected], npm/@nodelib/[email protected], npm/@nodelib/[email protected], npm/@nodelib/[email protected], npm/@pkgjs/[email protected], npm/@sinclair/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

Copy link

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteCI
Possible typosquat attack npm/[email protected] ⚠︎

View full report↗︎

Next steps

What is a typosquat?

Package name is similar to other popular packages and may not be the package you want.

Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

Copy link
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

LGTM - im happy with this. We need to come back to the UI side tho. I know we have cal V3 designs that we should match for this even if we havent updated or DS just yet

cc: @ciaranha

@keithwillcode keithwillcode merged commit 5d88858 into main Dec 5, 2024
38 checks passed
@keithwillcode keithwillcode deleted the eunjae/cal-4760-insights-text-filter-on-routing-list branch December 5, 2024 15:32
keithwillcode added a commit that referenced this pull request Dec 5, 2024
keithwillcode added a commit that referenced this pull request Dec 5, 2024
keithwillcode added a commit that referenced this pull request Dec 5, 2024
…18024)

* revert: "feat: refactor filters on insights response table (#17796)"

This reverts commit 5d88858.

* Put back the search columns and options english terms
keithwillcode added a commit that referenced this pull request Dec 5, 2024
…18024)

* revert: "feat: refactor filters on insights response table (#17796)"

This reverts commit 5d88858.

* Put back the search columns and options english terms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consumer core area: core, team members only insights area: insights, analytics ready-for-e2e 💻 refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants