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

Autocomplete: Only open menu on click #4771

Merged
merged 10 commits into from
Jul 29, 2024
Merged

Conversation

TylerJDev
Copy link
Contributor

@TylerJDev TylerJDev commented Jul 24, 2024

Closes https://github.com/github/primer/issues/3387

Sets openOnFocus to false by default.

On focus or click, we no longer show the menu by default. You may still activate the menu by entering Autocomplete.Input and pressing the up/down arrow keys. Showing the menu instantly upon focus means that a screen reader user is notified of the menu instantly with no indication given that the user is on the input itself.

Changelog

Changed

  • Sets openOnFocus to false

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Jul 24, 2024

🦋 Changeset detected

Latest commit: 36b8074

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

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

Copy link
Contributor

github-actions bot commented Jul 24, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 95.58 KB (-0.01% 🔽)
packages/react/dist/browser.umd.js 95.88 KB (-0.09% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4771 July 24, 2024 17:45 Inactive
@primer-integration
Copy link

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/334338

@TylerJDev TylerJDev marked this pull request as ready for review July 24, 2024 20:25
@TylerJDev TylerJDev requested a review from a team as a code owner July 24, 2024 20:25
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Would it be possible to use openOnFocus for this migration? This might be able to smooth things out and eventually remove this in v38 (let me know if you want me to add it to that list btw!) One approach could be to change the default to false and see how things go and eventually remove the prop altogether.

Curious what you think 👀

@@ -52,14 +52,14 @@ const AutocompleteInput = React.forwardRef(
const [highlightRemainingText, setHighlightRemainingText] = useState<boolean>(true)
const {safeSetTimeout} = useSafeTimeout()

const handleInputFocus: FocusEventHandler<HTMLInputElement> = useCallback(
const handleInputClick: MouseEventHandler<HTMLInputElement> = useCallback(
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ignore this since it's not really related to the changes proposed 👀

I wanted to ask if this useCallback is doing much for us or if it would be something we could refactor to a default event handler / function.

Copy link
Contributor Author

@TylerJDev TylerJDev Jul 25, 2024

Choose a reason for hiding this comment

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

Yeah we definitely can adjust this!

Edit: Would we want to do this for all the event handlers using useCallback?

@TylerJDev
Copy link
Contributor Author

Would it be possible to use openOnFocus for this migration?

So turning the prop to false by default so that both click and focus don't show the menu? We definitely could! I wasn't sure if there were instances where it was ideal for AutocompleteMenu to be visible upon click, but honestly I don't think it's the biggest risk.

@TylerJDev
Copy link
Contributor Author

@joshblack, I adjusted it so that we instead change openOnFocus to false by default! This means neither click/focus will trigger the menu. The user will have to manually go into the input and trigger it themselves. Let me know if any adjustments are needed!

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

LGTM! 🤞 it passed downstream 👀

Would it be helpful to add a @deprecated tag with this to the props and docs.json file, too?

@TylerJDev
Copy link
Contributor Author

TylerJDev commented Jul 29, 2024

Would it be helpful to add a @deprecated tag with this to the props and docs.json file, too?

Definitely! I added it to the prop & docs. I'll add it to the v38 release issue too!

@TylerJDev TylerJDev added this pull request to the merge queue Jul 29, 2024
Merged via the queue into main with commit 92e05f7 Jul 29, 2024
30 checks passed
@TylerJDev TylerJDev deleted the tylerjdev/autocomplete-on-click branch July 29, 2024 20:17
@primer primer bot mentioned this pull request Jul 30, 2024
TylerJDev added a commit that referenced this pull request Jul 31, 2024
* Only open menu on click instead of just focus

* Update tests

* Add changeset

* Fix typo

* Set `openOnFocus` to `true`

* Add test, move `onFocus` function

* Update docs

* Adjust changeset

* Remove `useCallback`

* Add deprecated notice
jonrohan added a commit that referenced this pull request Aug 8, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
* Focus close button on second step

* Remove `autofocus` prop

* Add dependencies to focus trap

* Add changeset

* lint fix

* Remove `body` from dependency

* add state to Dialog example

* chore(changeset): enter prerelease mode for v37 (#4789)

* chore(changeset): enter pre-release mode for v37

* ci: remove snapshots when in pre mode

* chore: add version info to all packages

* Revert "chore: add version info to all packages"

This reverts commit 4665bb3.

* chore: update canary to remove pre.json when running

---------

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

* Autocomplete: Only open menu on click (#4771)

* Only open menu on click instead of just focus

* Update tests

* Add changeset

* Fix typo

* Set `openOnFocus` to `true`

* Add test, move `onFocus` function

* Update docs

* Adjust changeset

* Remove `useCallback`

* Add deprecated notice

* Prep for high contrast theme border-color changes (#4774)

* borders

* fallback

* test(vrt): update snapshots

* test color changes

* alright

* bump

* test(vrt): update snapshots

* more fixes

* test(vrt): update snapshots

* copy from main

* test(vrt): update snapshots

* Create fluffy-ravens-thank.md

* snippy snaps

* update seg control border

* test(vrt): update snapshots

* remove fallbacks

* copy from main

* test(vrt): update snapshots

---------

Co-authored-by: langermank <langermank@users.noreply.github.com>

* chore: add package version numbers (#4796)

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

* feat: add postcss-preset-primer (#4751)

* feat: add postcss-preset-primer

* docs: update usage snippet

* chore: fix eslint rules and remove postcss-mixins

---------

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

* Utilize `aria-describedby` on all `ActionList` descriptions (#4666)

* Add `aria-describedby` to inline description; add `aria-labelledby` to `TrailingVisual`

* Update snapshots

* Add changeset

* Update snapshot

* Update packages/react/src/ActionList/Item.tsx

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

* changes from PR feedback

* Update .changeset/lovely-days-march.md

* Update snapshots

* Update lovely-days-march.md

---------

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

* Wrap `header` and `footer` with `<Box>`

* Move `<Box>`

* Add back focus to story

* Update behaviors package

* Move back `useFocusTrap`

---------

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>
Co-authored-by: Josh Black <joshblack@github.com>
Co-authored-by: Josh Black <joshblack@users.noreply.github.com>
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
Co-authored-by: langermank <langermank@users.noreply.github.com>
@primer primer bot mentioned this pull request Oct 18, 2024
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.

2 participants