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

[#1060] Adjust docs/typings for initialFocus option #1062

Merged
merged 2 commits into from
Sep 22, 2023
Merged

Conversation

stefcameron
Copy link
Member

Fixes #1060

The option can also be set to undefined or a function that returns undefined.

PR Checklist

Please leave this checklist in your PR.

  • Issue being fixed is referenced.
  • Source changes maintain stated browser compatibility.
  • Web APIs introduced have deep browser coverage, including Safari (often very late to adopt new APIs).
  • Includes updated docs demo bundle if source/docs code was changed (run npm run demo-bundle in your branch and include the /docs/demo-bundle.js file that gets generated in your PR).
  • Unit test coverage added/updated.
  • E2E (i.e. demos) test coverage added/updated.
    • ⚠️ Non-covered demos (look for // TEST MANUALLY comments here) that can't be fully tested in Cypress have been manually verified.
  • Typings added/updated.
  • Changes do not break SSR:
    • Careful to test typeof document/window !== 'undefined' before using it in code that gets executed on load.
  • README updated (API changes, instructions, etc.).
  • Changes to dependencies explained.
  • Changeset added (run npm run changeset locally to add one, and follow the prompts).
    • EXCEPTION: A Changeset is not required if the change does not affect any of the source files that produce the package bundle. For example, demo changes, tooling changes, test updates, or a new dev-only dependency to run tests more efficiently should not have a Changeset since it will not affect package consumers.

Fixes #1060

The option can _also_ be set to `undefined` or a function that returns
`undefined`.
@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2023

🦋 Changeset detected

Latest commit: 7d4024b

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

This PR includes changesets to release 1 package
Name Type
focus-trap Patch

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

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Unfortunately, that doesn't solve the problem. Being able to specify a function that always returns undefined is not really more helpful than specifying undefined directly. Instead, we would need something like FocusTargetValueOrFalse | (() => FocusTargetValueOrFalse | undefined) | undefined.

FYI, the information I get with your changes when trying initialFocus: () => someCondition ? someElement : undefined:

Type '() => HTMLElement | undefined' is not assignable to type 'FocusTargetOrFalse | (() => undefined) | undefined'.

@stefcameron
Copy link
Member Author

stefcameron commented Sep 21, 2023

@FloEdelmann Thanks for checking it out. You're right, I was able to reproduce the error. I think the fix is simply changing the function type to () => void. See this TypeScript playground where your test function works (code is at the bottom).

Copy link

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Indeed, that seems to work fine 🙂
Thanks for fixing this so quickly!

@stefcameron stefcameron merged commit 203b322 into master Sep 22, 2023
3 checks passed
@stefcameron stefcameron deleted the typings branch September 22, 2023 22:29
@stefcameron
Copy link
Member Author

I will push this out next early next week.

@stefcameron
Copy link
Member Author

Published in v7.5.3

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.

Adjust types to allow initialFocus: () => undefined
2 participants