ENG-1731: Add keyboard-only filtering with tag chips#1072
Conversation
Replace the advanced search input with a chip-based type filter input that supports ghost tab-completion and keyboard chip navigation while staying in sync with the dropdown filter state. Co-authored-by: Cursor <cursoragent@cursor.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 620c204db8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nodeTypes={discourseNodes} | ||
| onArrowDown={() => | ||
| setActiveIndex((index) => | ||
| Math.min(index + 1, results.length - 1), |
There was a problem hiding this comment.
Pressing ArrowDown in the chip input when results.length === 0 (for example, while indexing or after an empty search) computes Math.min(index + 1, -1), which sets activeIndex to -1. If results then appear without another term/filter/sort change, no row is selected and Enter/Cmd+Enter actions silently no-op until the user manually re-navigates. Clamp to a non-negative index or skip updates when there are no results.
Useful? React with 👍 / 👎.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A few things I noticed with this PR that I think we should address:
Input box alignment
It looks like this PR introduces a misalignment of the search box with the icons. I would consider this a regression that needs to be fixed:
Not using Blueprint Tags
Did we try using Blueprint Tag, TagInput, or InputGroup for this? Were there some issues stopping us from using the components that Roam style guide recommends?
Tailwind
As discussed in our last meeting, we noting how Roam doesn't handle all of the tailwind class syntax: specifically arbitrary values. So let's do a quick run through to double check.
| [nodeTypes], | ||
| ); | ||
|
|
||
| const selectedNodeTypes = useMemo( |
| const [focusedChipIndex, setFocusedChipIndex] = useState(-1); | ||
| const chipRefs = useRef<(HTMLSpanElement | null)[]>([]); | ||
|
|
||
| const nodeTypeById = useMemo( |
| [nodeTypes, searchTerm, selectedTypeIds], | ||
| ); | ||
|
|
||
| const completionSuffix = useMemo(() => { |
| </span> | ||
| </span> | ||
| )} | ||
| <input |
There was a problem hiding this comment.
Can we use blueprint TagInput or InputGroup for this?
| nodeType.canvasSettings?.color ?? "#000000", | ||
| )} | ||
| > | ||
| <span className="max-w-[10rem] truncate leading-4"> |
There was a problem hiding this comment.
Does Roam handle arbitrary width values?
| {nodeType.text} | ||
| </span> | ||
| <Button | ||
| className="!h-4 !min-h-0 !w-4 !min-w-0 !p-0" |
| {selectedNodeTypes.map((nodeType, index) => { | ||
| const isFocused = focusedChipIndex === index; | ||
| return ( | ||
| <span |
There was a problem hiding this comment.
Can we use blueprint tags for this?
| <span className="pointer-events-none absolute inset-0 flex items-center overflow-hidden whitespace-nowrap text-sm"> | ||
| <span className="invisible">{searchTerm}</span> | ||
| <span className="text-gray-400">{completionSuffix}</span> | ||
| <span className="ml-2 rounded bg-gray-100 px-1 text-[10px] uppercase tracking-wide text-gray-500"> |
There was a problem hiding this comment.
Does Roam handle text-[arbitrary value]?
2 participants
https://www.loom.com/share/dd5b9491a87b436395a5fb2344b51c24
Summary
Tabcommit behavior (prefix-only) and chip keyboard navigation/removalTest plan
Tabto commit a ghost completion into a chip and confirm search term clearsMade with Cursor