feat: add typings for hooks #3125
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
❌ Deploy Preview for paragon-openedx failed.Built without sensitive environment variables
|
bradenmacdonald
force-pushed
the
braden/type-hooks
branch
from
f30998a
to
fbb8045
Compare
|
OK, this is very weird. Without any the other changes, merely renaming |
| @@ -12,7 +12,7 @@ window.ResizeObserver = window.ResizeObserver | |||
| })); | |||
|
|
|||
| function TestComponent() { | |||
| const [containerElementRef, setContainerElementRef] = React.useState(null); | |||
| const [containerElementRef, setContainerElementRef] = React.useState<HTMLDivElement | null>(null); | |||
| const overflowElementRef = React.useRef(null); | |||
There was a problem hiding this comment.
nit/question: should both refs get the same type with HTMLDivElement, given both refs are applied to divs below?
| @@ -19,7 +20,7 @@ const resetHandlerMocks = () => { | |||
| }; | |||
|
|
|||
| // eslint-disable-next-line react/prop-types | |||
There was a problem hiding this comment.
nit/unrelated suggestion: could opt to disable prop-types rule for *.test.* files so we don't need to have these eslint disable comments throughout test files.
| function handleEnter( | ||
| { event, currentIndex, activeElement }: { event: KeyboardEvent, currentIndex: number, activeElement: HTMLElement }, | ||
| ) { |
There was a problem hiding this comment.
curious/suggestion: would it improve readability/understandability of these types if these function args and the return data structure were abstracted into an interface? E.g., something like the following:
interface HandleEnterOptions {
event: KeyboardEvent;
currentIndex: number;
activeElement: HTMLElement;
}
function handleEnter({ event, currentIndex, activeElement }: HandleEnterOptions) { ... }Similar question for handleArrowKey below
| containerElementRef: Element | null, | ||
| overflowElementRef: Element | null, |
There was a problem hiding this comment.
nit/clarification: should these Element be HTMLElement?
| isVisible: boolean, | ||
| sentinelRef: React.MutableRefObject<HTMLDivElement | null>, | ||
| ] => { | ||
| const sentinelRef = useRef<HTMLDivElement | null>(null); |
There was a problem hiding this comment.
[clarification] Will this sentinelRef always be applied to a Div element? Might it be used with non-div elements, too?
Description
This PR adds typescript types for the various hooks that Paragon provides:
useWindowSize,useToggle,useArrowKeyNavigation,useIndexOfLastVisibleChild, anduseIsVisible. There should be no changes other than to the typings.Deploy Preview
Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).
Merge Checklist
n/a - doesn't apply to this type of PR.
Post-merge Checklist