Skip to content

v2 Menus: Implement typeahead focus - #1834

Merged
siddharthkp merged 13 commits into
mainfrom
siddharth/menu-focus-first-letter
Feb 21, 2022
Merged

v2 Menus: Implement typeahead focus#1834
siddharthkp merged 13 commits into
mainfrom
siddharth/menu-focus-first-letter

Conversation

@siddharthkp

@siddharthkpsiddharthkp commented Feb 3, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Looking for feedback on:

  1. Is this correct behavior - Typeahead does not activate automatically when you open with mouse click. But, if you press the arrow keys after opening with mouse, typeahead becomes activated.
  2. Is the typeahead timeout 1000ms appropriate? The timeout is necessary so that you can type a longer string to match options. At the same, if you have found the matching element quickly before the timeout runs wouldn't select the option, making it feel unresponsive. See storybook setup to play around with it.

Screenshots:

Before:
Pressing alphabet keys does nothing

After:

Pressing alphabet keys changes focus the matching element

100% test coverage yo!

image

@changeset-bot

changeset-botBot commented Feb 3, 2022•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4ff95ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactMinor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@siddharthkpsiddharthkp self-assigned this Feb 3, 2022
@github-actions

github-actionsBot commented Feb 3, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js62.62 KB (0%)
dist/browser.umd.js63.01 KB (0%)

@siddharthkp
siddharthkp marked this pull request as ready for review February 3, 2022 15:45
@siddharthkp
siddharthkp requested a review from a teamFebruary 3, 2022 15:45
@siddharthkpsiddharthkp changed the title Menu: Implement typeaheadMenus: Implement typeaheadFeb 3, 2022
@siddharthkpsiddharthkp changed the title Menus: Implement typeaheadv2 Menus: Implement typeaheadFeb 3, 2022
@siddharthkpsiddharthkp changed the title v2 Menus: Implement typeaheadv2 Menus: Implement typeahead focusFeb 3, 2022
siddharthkp
siddharthkp commented Feb 3, 2022
View reviewed changes
Comment threadsrc/ActionMenu2.tsx
const {containerRef, openWithFocus} = useMenuInitialFocus(open, onOpen)
const containerRef = React.createRef<HTMLDivElement>()
const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
useTypeaheadFocus(open, containerRef)

@siddharthkpsiddharthkpFeb 3, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Note 1: Changed both of these hooks to accept a ref if it's passed (or create a new one if it's not passed)

Note 2: Did not combine these into one hook called useMenuFocus, we might want to re-use typeahead logic in NavigationTree 🤷

@siddharthkp
siddharthkp requested review from alliethu and removed request for alliethuFebruary 4, 2022 12:32
@siddharthkpsiddharthkp added this to the FY22 - Q3 milestone Feb 4, 2022
@colebemis

colebemis commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

Is this correct behavior - Typeahead does not activate automatically when you open with mouse click. But, if you press the arrow keys after opening with mouse, typeahead becomes activated.

I'm also curious about this ☝️ cc @alliethu

It seems a little strange that typeahead doesn't work when activated with a mouse, but I don't have a strong opinion.

colebemis
colebemis approved these changes Feb 8, 2022
View reviewed changes

@colebemiscolebemis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work @siddharthkp! Looks good to me 👍 I would like an answer to #1834 (comment) from someone with more accessibility experience before we merge this, but feel free to merge if this is urgent.

Side note, this would be fun to implement as a state machine!

@siddharthkp

siddharthkp commented Feb 15, 2022

Copy link
Copy Markdown
MemberAuthor

This is an interesting update which would change the behavior here as well :) primer/behaviors#52

Will wait for behaviors release before moving forward on this PR

@siddharthkp

siddharthkp commented Feb 21, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Update: After #1877 (which is also merged in this branch), typeahead is active even when you open the menu with mouse click ✌️

@siddharthkp
siddharthkp removed the request for review from alliethuFebruary 21, 2022 16:02
@siddharthkp
siddharthkp enabled auto-merge (squash) February 21, 2022 16:10
@siddharthkp
siddharthkp merged commit 2abd7b7 into mainFeb 21, 2022
@siddharthkp
siddharthkp deleted the siddharth/menu-focus-first-letter branch February 21, 2022 16:15
@primer-cssprimer-css mentioned this pull request Feb 21, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@colebemiscolebemiscolebemis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@siddharthkpsiddharthkp

@colebemiscolebemis

Labels

accessibilityreact

Projects

None yet

Milestone

FY22 - Q3

Development

Successfully merging this pull request may close these issues.

2 participants

@siddharthkp@colebemis