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

fix: Better filtering by ref values #2563

Merged
merged 10 commits into from
Oct 2, 2024

Conversation

tssweeney
Copy link
Collaborator

@tssweeney tssweeney commented Oct 1, 2024

Filtering by values within a reffed object is technically quite hard. However, Scott made the great recommendation today to have the following rules:

  1. Don't allow the user to provide free-text input to columns which are expanded columns
  • As a result, this means that users will not be able to accidentally filter by something that isn't supported
  1. If a user option-clicks a field, use the parent ref as the filter, rather than trying to filter by the value
  • As a result, users will still feel like they can effectively filter the results. Note: if two distinct objects have the same value for a given property, the filter will only produce rows where the ref corresponds to the clicked cell. This is likely still going to be a friction point.

This Pr does those two things (:

@tssweeney tssweeney requested review from a team as code owners October 1, 2024 23:52
@tssweeney tssweeney marked this pull request as draft October 1, 2024 23:52
@circle-job-mirror
Copy link

circle-job-mirror bot commented Oct 1, 2024

@tssweeney tssweeney marked this pull request as ready for review October 2, 2024 01:25
if (columnIsRefExpanded(field)) {
// In this case, we actually just want to filter by the parent ref itself.
// This means we need to:
// 1. Determine the column of the highest level anscestor column with a ref
Copy link
Collaborator

Choose a reason for hiding this comment

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

anscestor => ancestor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

// 1. Determine the column of the highest level anscestor column with a ref
// 2. Get the value of that corresponding cell (ref column @ row)
// 3. Add a filter for that ref on that column.
// The aknoweldged drawback of this approach is that we are not filtering by that
Copy link
Collaborator

Choose a reason for hiding this comment

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

aknoweldged => acknowledged

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

let ancestorField: string | null = null;
let targetRef: string | null = null;
for (let i = 0; i <= fieldParts.length; i++) {
const ancestorFieldCandidate = fieldParts.slice(0, i).join('.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

slice(0,0) will be an empty list, I think we don't need to check that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@@ -306,6 +373,18 @@ export const CallsTable: FC<{
onAddFilter
);

// This contains columns which are suitable for selection and raw data
// entry. Noteably, not children of expanded refs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noteably => Notably

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@tssweeney tssweeney merged commit bf1a3b2 into master Oct 2, 2024
78 of 79 checks passed
@tssweeney tssweeney deleted the tim/filter_by_ref_temp_work_around branch October 2, 2024 23:16
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants