-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=9d3af881ec86e2f60b78fa5d6995364a26babbdc |
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 |
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.
anscestor => ancestor
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.
👍
// 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 |
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.
aknoweldged => acknowledged
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.
👍
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('.'); |
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.
slice(0,0)
will be an empty list, I think we don't need to check that case?
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.
👍
@@ -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. |
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.
Noteably => Notably
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.
👍
Filtering by values within a reffed object is technically quite hard. However, Scott made the great recommendation today to have the following rules:
This Pr does those two things (: