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

fix: fixed search bar layout and ux: can now click to preview search results #27015

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

adamleithp
Copy link
Contributor

@adamleithp adamleithp commented Dec 18, 2024

Loom: https://www.loom.com/share/e709b159112b42cb991d3a302424b825?sid=d4dde375-3c7d-49df-a805-39635f4f279a

Problem

I found the UX of the search bar hard to understand:

  • Search results were not clickable (mouse), and the preview of the result was only visible when navigating with keyboard.
  • Search input now has focus-visible outline styles.

I found the layout of the search bar:

  • Mobile: not-friendly
  • Desktop: To break slightly when previewing long un-breaking strings.
  • Updated layout of search bar with a more rigid grid layout, which is mobile first.

Changes

  • Updated DOM layout of search bar with grid. Better mobile view.
  • Search bar now has focus-visible outline styles.
  • Desktop:
    • clicking search results allow you to preview the item (was previously only available when using keyboard)
    • Show keyboard ENTER keyboard shortcut to express how you jump to search item page. Mobile still acts the same (click goes to page)

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Opened on major browsers (both keyboard functionality and clicking), tested mobile

…sktop, new layout results in less janky keyboard navigation
@adamleithp adamleithp requested a review from Twixes December 18, 2024 15:10
@corywatilo
Copy link
Contributor

👏 Yesssss.

Only feedback: What if we moved the keyboard shortcut inside the button so we don't duplicate the text? Like:

Open ⏎

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Size Change: 0 B

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.11 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@adamleithp adamleithp removed the request for review from Twixes December 18, 2024 15:29
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Great improvements, just finding the focus-visible styling odd, and +1 to Cory's point

@adamleithp adamleithp force-pushed the fix/search-bar-ux-and-layout branch from 22e00c8 to f6d2a1e Compare December 18, 2024 23:57
@adamleithp
Copy link
Contributor Author

adamleithp commented Dec 19, 2024

👏 Yesssss.

Only feedback: What if we moved the keyboard shortcut inside the button so we don't duplicate the text? Like:

Open ⏎

@corywatilo

image

@corywatilo
Copy link
Contributor

I generally like it! Wdyt? (I think we can tweak later but imo 🚀?)

@adamleithp adamleithp requested a review from daibhin December 19, 2024 00:12
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@adamleithp adamleithp requested review from Twixes and removed request for daibhin December 19, 2024 06:34
@adamleithp adamleithp enabled auto-merge (squash) December 19, 2024 06:36
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

This is nice

@adamleithp adamleithp merged commit 5e97bb3 into master Dec 19, 2024
99 checks passed
@adamleithp adamleithp deleted the fix/search-bar-ux-and-layout branch December 19, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants