Repository navigation
[HDX-5307] Drop the search bar WHERE label and / keycap #3168
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@hyperdx/app': patch | ||
| --- | ||
|
|
||
| fix: drop the search bar WHERE label and `/` keycap overlay | ||
|
|
||
| The `WHERE` label repeated the SQL placeholder, and the `/` hint clipped long queries. `/` and `s` still focus the input; the overlay is gone. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,13 +79,17 @@ describe('SearchWhereInput', () => { | |
|
|
||
| describe('Lucene Mode', () => { | ||
| it('renders Lucene input when whereLanguage is lucene', () => { | ||
| renderWithMantine(<TestWrapper defaultLanguage="lucene" />); | ||
| const { container } = renderWithMantine( | ||
| <TestWrapper defaultLanguage="lucene" />, | ||
| ); | ||
|
|
||
| // Lucene mode uses a textarea from AutocompleteInput | ||
| const input = screen.getByPlaceholderText( | ||
| /Search your events w\/ Lucene/i, | ||
| ); | ||
| expect(input).toBeInTheDocument(); | ||
| // `/` and `s` still focus the input; the overlay keycap is gone. | ||
| expect(container.querySelector('kbd')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('allows typing in Lucene mode', async () => { | ||
|
|
@@ -135,10 +139,15 @@ describe('SearchWhereInput', () => { | |
| }); | ||
|
|
||
| describe('SQL Mode', () => { | ||
| it('renders SQL input with WHERE label when whereLanguage is sql', () => { | ||
| it('renders the SQL editor when whereLanguage is sql', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — The renamed SQL-mode test no longer pins the removal of the WHERE label The two assertions that covered the label ( Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| renderWithMantine(<TestWrapper defaultLanguage="sql" />); | ||
|
|
||
| expect(screen.getByText('WHERE')).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByRole('combobox', { name: 'Query language' }), | ||
| ).toHaveValue('SQL'); | ||
| expect( | ||
| screen.queryByPlaceholderText(/Search your events w\/ Lucene/i), | ||
| ).not.toBeInTheDocument(); | ||
|
Comment on lines
+142
to
+150
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The rewritten SQL-mode test confirms that the SQL editor renders, but it no longer checks the behavior this PR changes: that the Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| }); | ||
|
|
||
| it('renders SQL placeholder', () => { | ||
|
|
@@ -196,23 +205,6 @@ describe('SearchWhereInput', () => { | |
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('hides label when showLabel is false', () => { | ||
| renderWithMantine( | ||
| <TestWrapper defaultLanguage="sql"> | ||
| {({ control }) => ( | ||
| <SearchWhereInput | ||
| tableConnection={mockTableConnection} | ||
| control={control} | ||
| name="where" | ||
| showLabel={false} | ||
| /> | ||
| )} | ||
| </TestWrapper>, | ||
| ); | ||
|
|
||
| expect(screen.queryByText('WHERE')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('uses custom placeholders when provided', () => { | ||
| renderWithMantine( | ||
| <TestWrapper defaultLanguage="lucene"> | ||
|
|
||
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.
🔵 minor — The new assertion pins the keycap's absence, not the hotkey the PR claims still works
expect(container.querySelector('kbd')).not.toBeInTheDocument()is an absence proxy, and the comment beside it ("/andsstill focus the input") is the one thing nothing checks — a grep forpress('/')/keyboard('/')/hotkeyacrosspackages/app/testsand the jest suites returns nothing, so deleting theuseHotkeys(['/', 's'], …)call inSearchInputV2.tsx:128orSQLEditor/SQLInlineEditor.tsx:228keeps every test green while breaking the behaviour the changeset promises. Add a case that renders withenableHotkey, fires/ondocument.body, and asserts the Lucene textarea (and the SQL.cm-content) has focus.Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond