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

Adjust types to allow initialFocus: () => undefined #1060

Closed
FloEdelmann opened this issue Sep 20, 2023 · 4 comments · Fixed by #1062
Closed

Adjust types to allow initialFocus: () => undefined #1060

FloEdelmann opened this issue Sep 20, 2023 · 4 comments · Fixed by #1062

Comments

@FloEdelmann
Copy link

initialFocus can be undefined, in this case the first tabbable node will be focused. Also, we can pass a function returning an element. But we can't return undefined in that function (according to the types):

export type FocusTarget = FocusTargetValue | (() => FocusTargetValue);

But according to the implementation, it actually works:

let node = getNodeForOption('initialFocus');

focus-trap/index.js

Lines 233 to 267 in 94645eb

const getNodeForOption = function (optionName, ...params) {
let optionValue = config[optionName];
if (typeof optionValue === 'function') {
optionValue = optionValue(...params);
}
if (optionValue === true) {
optionValue = undefined; // use default value
}
if (!optionValue) {
if (optionValue === undefined || optionValue === false) {
return optionValue;
}
// else, empty string (invalid), null (invalid), 0 (invalid)
throw new Error(
`\`${optionName}\` was specified but was not a node, or did not return a node`
);
}
let node = optionValue; // could be HTMLElement, SVGElement, or non-empty string at this point
if (typeof optionValue === 'string') {
node = doc.querySelector(optionValue); // resolve to node, or null if fails
if (!node) {
throw new Error(
`\`${optionName}\` as selector refers to no known node`
);
}
}
return node;
};

So I suggest to adjust the types to allow undefined as a valid function return type.

@stefcameron
Copy link
Member

@FloEdelmann You're correct! Setting the option to undefined or a function that returns undefined is specifically handled in getInitialFocusNode() so the docs and typings should reflect that. Thank you for bringing this to my attention.

I'll get this sorted shortly!

stefcameron added a commit that referenced this issue Sep 21, 2023
Fixes #1060

The option can _also_ be set to `undefined` or a function that returns
`undefined`.
stefcameron added a commit that referenced this issue Sep 21, 2023
Fixes #1060

The option can _also_ be set to `undefined` or a function that returns
`undefined`.
@stefcameron
Copy link
Member

@FloEdelmann Please have a look at #1062 and LMK if the typings look good now.

stefcameron added a commit that referenced this issue Sep 22, 2023
[#1060] Adjust docs/typings for initialFocus option

Fixes #1060

The option can _also_ be set to `undefined` or a function that returns
`undefined`.
@stefcameron
Copy link
Member

Fix published in v7.5.3

@FloEdelmann
Copy link
Author

Thanks!

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 a pull request may close this issue.

2 participants