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

[CommandBar] Enable input & tab pre-population on open #1800

Merged
merged 4 commits into from Mar 31, 2023

Conversation

ashleemboyer
Copy link
Contributor

@ashleemboyer ashleemboyer commented Mar 30, 2023

πŸ”— Relevant links

πŸ—’οΈ What

  • Adds basic <button>s to the featured search card
    • The featured search terms are defined in the attached task
    • These buttons leverage the setCurrentInputValue and toggleIsOpen helpers exposed by CommandBar
  • Sets the contentType of the HomePageView to 'tutorials'
    • This makes CommandBar show the "Tutorials" tab by default
    • Added a comment with explicit context, since we previously had this page use the "global" contentType
  • Adds a magenta border around WIP cards, since this page is starting to shape up and may look official/done/broken otherwise

πŸ§ͺ Testing

  • Go to the home page on the preview
  • For each <button> in the "Search with ease" featured card:
    • Activate the button
    • CommandBar should open
    • The input text should match the button's text
    • The Tutorials tab should be selected

πŸ’­ Anything else?

  • This PR does not style these new buttons. That will come in a separate task currently scheduled for next week: Style & animate SearchCard.
  • There's a slight delay/flash when CommandBar is opened with these buttons. This is because SearchCommandBarDialogBody waits to send off search queries for 300ms after the text input is finished changes, and because of the search results loading delay. This isn't ideal, and I'm thinking through a couple of options for optimizing this before release. This PR will not optimize this.
    • Expose the setSearchQuery helper from SearchCommandBarDialogBody. This doesn't feel super ideal because it's not intended to be used as a utility, it's still a feasible and low-effort option. However, it will only remove the 300ms before the search query is sent off. It will not help the results loading delay (an existing issue).
    • Pre-load the search results for these buttons on render, and put the results in the React Query cache. Since we currently rely solely on Algolia's UI components/contexts to invoke search results fetching, I'm not sure how feasible this idea is. I'll do some digging and see if Algolia exposes any prefetching utilities though!

@vercel
Copy link

vercel bot commented Mar 30, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated
dev-portal βœ… Ready (Inspect) Visit Preview πŸ’¬ Add your feedback Mar 30, 2023 at 2:04PM (UTC)

@github-actions
Copy link

Some suggested prefixes and emojis that may help to write clear, actionable code review comments:

Praise πŸ™Œ Question πŸ™‹ Thought πŸ’­ Blocker 🚧 Future πŸ“Œ Optional 🎨 Nitpick ⛏️
Expand for comment prefix descriptions
Prefix+Emoji Description
Praise πŸ™Œ Use to highlight something positive. It's nice to try to leave one per review, but don't leave false praise just to leave one of these comments.
Question πŸ™‹ Use to gain clarity from the code author. The conversation could evolve into any one of these other categories. Only the reviewer should resolve these comment threads.
Thought πŸ’­ Use to share context, leave a breadcrumb, or share an idea that came up while reviewing.
Blocker 🚧 Use to request changes that block merging the current PR. Only the reviewer should resolve these comment threads.
Future πŸ“Œ Use to request changes that the code author can choose to address in the current PR or a follow-up one.
Optional 🎨 Use to suggest optional changes that you feel strongly about but ultimately defer to the code author to make a decision on. These can be comments that turn into valuable conversation starters for adopting new code styles, guidelines, or team practices.
Nitpick ⛏️ Use to suggest changes based on loose opinions or personal preferences. The difference between this and Optional 🎨  is how strong the code reviewer's opinion is.

@github-actions
Copy link

github-actions bot commented Mar 30, 2023

πŸ“¦ Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action πŸ€–

This PR introduced no changes to the javascript bundle πŸ™Œ

@ashleemboyer ashleemboyer changed the title [WIP] [CommandBar] Enable input & tab pre-population on open [CommandBar] Enable input & tab pre-population on open Mar 30, 2023
@ashleemboyer ashleemboyer marked this pull request as ready for review March 30, 2023 14:17
@ashleemboyer ashleemboyer requested a review from a team as a code owner March 30, 2023 14:17
@ashleemboyer
Copy link
Contributor Author

@BRKalow no review required here, but if you've got time to take a peek at the "Anything else?" section of the PR description, I'd appreciate any breadcrumbs you might have about Algolia magic! 🀩

Copy link

@emilypersson1 emilypersson1 left a comment

Choose a reason for hiding this comment

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

working perfectly, thanks Ashlee!

Copy link
Member

@zchsh zchsh left a comment

Choose a reason for hiding this comment

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

βœ… Followed testing steps, this looks great to me!

@zchsh zchsh self-requested a review March 30, 2023 20:15
@BRKalow
Copy link
Contributor

BRKalow commented Mar 30, 2023

I think Algolia has some internal caching, so we could potentially fire off a search query ahead of time to "warm the cache" independent of any react-query usage.

The debounce is tricky πŸ€”. I think the implementation you've got here is the right one (using the generic command bar utility).

Perhaps we could check if the input value is non-empty on first render, and if so set the search query right away. The debounce would only come into play on subsequent updates then.

Copy link
Contributor

@kendallstrautman kendallstrautman left a comment

Choose a reason for hiding this comment

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

LGTM! Your options in the description to explore caching seem like good avenues

@ashleemboyer ashleemboyer added this pull request to the merge queue Mar 31, 2023
Merged via the queue into main with commit 94c0ffc Mar 31, 2023
9 checks passed
@ashleemboyer ashleemboyer deleted the amb.cb-settings-on-open branch March 31, 2023 00:21
@ashleemboyer
Copy link
Contributor Author

@zchsh @BRKalow @kendallstrautman just a heads up about the follow-up optimization work -- I created an Asana task co-located with my other tasks for this project!

🎟️ Explore options for eliminating the flash of the Global Search buttons in the search card

The TL;DR is -- timebox 4 hours to explore the current 4 ideas we have, report the findings, and propose 1 to the team (just via Slack probably) for discussion.

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.

None yet

5 participants