feat(compass-crud): add 'Add to query' to the element context menu COMPASS-9394 #7091
Conversation
gagik
changed the title
packages/compass-components/src/components/document-list/element.tsx
Outdated
Show resolved
Hide resolved
| { | ||
| label: 'Add to query', | ||
| onAction: () => { | ||
| onAddToQuery(key.value, element.generateObject()); |
There was a problem hiding this comment.
Not sure that for nested queries it works as expected, this filter I got for one of the genres item is kinda not meaningful (same for nested objects):
We probably want to build the whole key when adding something to the filter, similar to how schema visualizer does it:
gagik
force-pushed
the
gagik/add-to-query-menu
branch
from
63660e1 to
b0d3c3f
Compare
| * Helper function to get the nested key path of an element, skips array keys | ||
| * Meant for keypaths used in query conditions from a selected element | ||
| */ | ||
| const getNestedKeyPath = (element: HadronElementType): string => { |
There was a problem hiding this comment.
Not sure if this fully replicates what was meant with schema visualization but this approach seems to work, the only caveat here is that with arrays one can only add to query 1 nested item at a time.
Alternatively we can make it specify the index (which would enforce its position.... not sure how common that is) or manually handle it to be smarter about it.
IMO, as a first iteration one item a time isn't too bad. We can have multiple nested fields for the object in the meantime so that's good
There was a problem hiding this comment.
We already handle this in schema tab with toggleDistinctValue case (or as we only have "add" here, you can look at addDistinctValue), I think for an "add to query" option it makes more sense than overriding the existing value
| [changeQuery] | ||
| ); | ||
|
|
||
| const isInQuery = useCallback( |
There was a problem hiding this comment.
@gribnoysup toggling seems to work pretty well so I changed the experience to also let us remove from query accordingly.
The only corner case I noticed is trying to add to query the whole array (i.e. without expanding) and then trying to add a specific element. This seems like something that'd need to be handled differently at toggleDistinctValue level I'd guess?
Not sure if the prop passing here is getting too much. I was thinking of at least combining them into a single queryController (or better naming...) object that would have add and contains to combine these 2 callbacks.
| const queryBar = useMemo<QueryBarController>(() => { | ||
| return { | ||
| isInQuery: (field: string, value: unknown) => { | ||
| const filter = queryBarQuery.filter?.[field]; | ||
| return hasDistinctValue(filter, value); | ||
| }, | ||
| toggleQueryFilter: (field: string, value: unknown) => { | ||
| queryBarChangeQuery('toggleDistinctValue', { | ||
| field, | ||
| value, | ||
| }); | ||
| }, | ||
| }; | ||
| }, [queryBarChangeQuery, queryBarQuery]); |
There was a problem hiding this comment.
Feel free to tell me "no" on this, but I think we might want to stick with more common React patterns here, at least I don't see much reason for this controller interface over just props (but again, I'm not strongly against what you have here), I think in the long run it might make it tough to connect the dots in the code. Some thoughts in no particular order:
- Usually passing down a getter like
isInQuerythat you have here is done for control flow inversion purposes (that usually related to some tricky rendering flows) where the child has more control over rendering that you don't have (likegetItemIndexin react-window for example). As we're not doing anything like that here I think you might as well just pass the whole query to the document and calculate theisInQueryvalue in the Element render instead. - If you do want to optimize this (which is maybe not a bad idea so that we don't re-render the whole document list while user types a query), you will need to go a different way about this, the function here needs to be completely stable for this to work, so some sort of query ref instead of query state will be needed. But that's also maybe a reason to just do "add to query" for the moment and avoid complicating this too much.
- The
toggleQueryFilteris really just a normal callback prop, so I'd suggest to stick with it and make it into optionalonUpdateQueryClick(or something similar) prop directly on the component, I think that's what you had before here and it honestly makes a lot of sense as an interface
There was a problem hiding this comment.
agreed all around, I'm still going to keep Remove query wherever possible since I'd have to handle the "toggle" logic one way or another or introduce a new add-only action to the change to query helper function. I think as it stands the removal is quite useful and works well enough
This reverts commit 19c87f9.
gagik
force-pushed
the
gagik/add-to-query-menu
branch
from
ca63796 to
a616886
Compare
| } = useHadronElement(element); | ||
|
|
||
| // Function to check if a field is in the query | ||
| // TODO: COMPASS-9541 Improve the functionality when checking for nested objects. |
There was a problem hiding this comment.
| export const hasDistinctValue = ( | ||
| field: { $in: any } | undefined, | ||
| value?: any | ||
| field: Record<string, unknown> | undefined, |
There was a problem hiding this comment.
drive-by fix as the types didn't seem right
gagik
changed the title
packages/compass-components/src/components/document-list/element.tsx
Outdated
Show resolved
Hide resolved
|
Looks good, however I noticed one more issue when adding query. I've attached video Screen.Recording.2025-07-09.at.11.32.56.mov |
Okay, we have a follow up on this specific issue: COMPASS-9541. |
|
Product confirmed the edge cases are minor and we can follow up on this behavior with COMPASS-9541, going ahead with merge. |
Adds an "Add to query" and "Remove to query" for adding and removing conditions from query with right click
