refactor: Remove INavigable in favor of IFocusableNode. #9037
Conversation
There was a problem hiding this comment.
Thanks @gonfunko! Had some thoughts.
Approving to unblock, but I'm happy to take another pass. Didn't look at the flyout navigation policy super closely since it appeared to have a bunch of unfinished code int.
| * the given object. | ||
| * | ||
| * @param current An instance to check whether this policy applies to. | ||
| * @returns True if the given object is of a type handled by this policy. |
There was a problem hiding this comment.
A nit but I don't think this actually returns a boolean, I think it returns current coerced to type T or undefined if coercion fails per the predicate.
I'm not 100% certain on this and I find the TypeScript documentation unclear, but I recall debugging guards like this before and being surprised that their return values weren't actually booleans.
There was a problem hiding this comment.
I agree the docs are unclear on this, but I think boolean is more useful to both folks reading and implementing than the perhaps technically correct value; ultimately the method needs to return a boolean (if you're implementing it) and you can rely on it being a truthy value if you call it.
There was a problem hiding this comment.
I used the language 'whether' in this case which I suppose is slightly more ambiguous (perhaps beneficially):
/**
* Determines whether the provided object fulfills the contract of
* IFocusableNode.
*
* @param object The object to test.
* @returns Whether the provided object can be used as an IFocusableNode.
*/
export function isFocusableNode(object: any | null): object is IFocusableNode {
return (
object &&
'getFocusableElement' in object &&
'getFocusableTree' in object &&
'onNodeFocus' in object &&
'onNodeBlur' in object &&
'canBeFocused' in object
);
}Not sure that's actually better. Note I don't have strong opinions here, so if you prefer the explicit 'True' that seems reasonable to me.
The basics
The details
Proposed Changes
This PR gets rid of the
INavigableinterface, since it was reduced to justgetClass(), and there was significant objection to that. Instead, we now act directly onIFocusableNodeinstances. As a side effect, this also makes navigation policies Just Work forFieldsubclasses, and opens up the possibility of removing the cursor'scurNodein favor of asking the focus manager for the currently focused thing, although I'll address that in a followup change. Registering navigation policies has been moved out of the cursor and into theNavigator.