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

replace search bar with command palette toggle #42603

Merged
merged 6 commits into from
May 15, 2024

Conversation

npfitz
Copy link
Contributor

@npfitz npfitz commented May 13, 2024

Closes #42346

Description

PR swaps out the search bar for a button in the AppBar component when we are not embedded. Button should toggle the command palette. Tests were updated to either

  1. Test in embedding mode when we were testing functionality of the floating search results or
  2. Perform the search via the command palette

How to verify

  1. click the Search button on the top of the screen. It should open the command palette
  2. should also work in mobile view
  3. test that embedding still shows original search bar

Demo

chrome_bFHMwf0qUl

Checklist

  • Tests have been added/updated to cover changes in this PR

@npfitz npfitz added the no-backport Do not backport this PR to any branch label May 13, 2024
@npfitz npfitz requested a review from a team May 13, 2024 20:19
@npfitz npfitz self-assigned this May 13, 2024
@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label May 13, 2024
Copy link

replay-io bot commented May 13, 2024

Status Complete ↗︎
Commit d6fcad9
Results
⚠️ 4 Flaky
2504 Passed

Copy link
Contributor

@iethree iethree left a comment

Choose a reason for hiding this comment

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

Implementation looks good.

I still think the button looks funny. 🤷‍♂️

@npfitz npfitz force-pushed the 42346-command-palette-search-button branch from 3a774f6 to 468ec6c Compare May 15, 2024 14:04
@npfitz npfitz requested a review from camsaul as a code owner May 15, 2024 14:04
@@ -202,9 +202,11 @@ function testOfficialBadgeInSearch({
question,
expectBadge,
}) {
appBar().findByPlaceholderText("Search…").as("searchBar").type(searchQuery);
// appBar().findByPlaceholderText("Search…").as("searchBar").type(searchQuery);
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

@iethree iethree self-requested a review May 15, 2024 18:54

["admin", "normal"].forEach(user => {
describe(`search > ${user} user`, () => {
beforeEach(() => {
restore();
cy.signIn(user);
cy.visit("/");
// cy.visit("/");
Copy link
Contributor

Choose a reason for hiding this comment

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

🗑️

@npfitz npfitz merged commit f37c417 into master May 15, 2024
111 checks passed
@npfitz npfitz deleted the 42346-command-palette-search-button branch May 15, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/AdminWebapp Admin and Webapp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace the search bar with a search button that opens the command palette
3 participants