Typescript and docs #794
Typescript and docs #794
Conversation
There was a problem hiding this comment.
Insane, great job!
How useful are actually prop types if you have typescript? To give the user better feedback about his props probably so still useful.
Quite hard to review, because it's quite big. Are there a lot of API changes? seems not what I've saw, then once the tests are passing I think we are ready to go. Incredible work!
| @@ -0,0 +1,8 @@ | |||
| export interface TransitionCallbacks { | |||
| onEnter?(node: HTMLElement): any; | |||
There was a problem hiding this comment.
void is more correct, but any allows folks to do early returns like return callSomething() slightly more flexible and less descriptive. TBH tho i just copied this type from RB
Hopefully not! Should just be type additions where there weren't any |
|
👍 / 👎 ? |
| @@ -69,7 +69,7 @@ const propTypes = { | |||
| * A callback fired when the Dropdown wishes to change visibility. Called with the requested | |||
| * `show` value, the DOM event, and the source that fired it: `'click'`,`'keydown'`,`'rootClose'`, or `'select'`. | |||
| * | |||
| * ```js | |||
| * ```ts static | |||
| { show: 'onToggle' }, | ||
| const [show, onToggle] = useUncontrolledProp( | ||
| rawShow, | ||
| defaultShow!, |
There was a problem hiding this comment.
non-null assertion here seems somewhat wrong? this is potentially legitimately null-y, no?
There was a problem hiding this comment.
It can, the bang is just to make the type correct without specifying the generic
| { show: 'onToggle' }, | ||
| const [show, onToggle] = useUncontrolledProp( | ||
| rawShow, | ||
| defaultShow!, |
There was a problem hiding this comment.
non-null assertion here seems somewhat wrong? this is potentially legitimately null-y, no?
| drop?: DropDirection; | ||
| }; | ||
|
|
||
| const DropdownContext = React.createContext<DropdownContextValue | null>(null); |
There was a problem hiding this comment.
might be safer to do null as any? i mean, we don't really deal with the case where the context isn't available, right? no point being robust to that
There was a problem hiding this comment.
needed for examples and things where we just show e.g. the Dropdown Menu
| import { createPopper } from './popper'; | ||
|
|
||
| const initialPopperStyles = { | ||
| const initialPopperStyles: Partial<CSSStyleDeclaration> = { |
There was a problem hiding this comment.
shouldn't this be the react css properties?
There was a problem hiding this comment.
there is an annoying incompatibility there with what Popper typed this as and what react does
src/DropdownToggle.tsx
Outdated
| toggle: DropdownContextValue['toggle']; | ||
| } | ||
|
|
||
| const noop: any = () => {}; |
There was a problem hiding this comment.
this matches the typing, no? do we need the explicit any type?
| if (canUseDOM && !prevShow && show) { | ||
| lastFocusRef.current = activeElement() as HTMLElement; | ||
| } |
There was a problem hiding this comment.
is it really safe to access the DOM during render? should this not go in useLayoutEffect?
There was a problem hiding this comment.
an effect is too late, even layoutEffect. I believe this is safe, but there isn't actually a getSnapshot lifecycle hook equiv. Also we do this in Dropdown and it's been fine so far
src/Modal.tsx
Outdated
| // Show logic when: | ||
| // - show is `true` _and_ `container` has resolved | ||
| useEffect(() => { | ||
| if (!show || !container) return; | ||
|
|
||
| handleShow(); | ||
| }, [show, container, /* show never change: */ handleShow]); | ||
|
|
||
| // Hide cleanup logic when: | ||
| // - `exited` switches to true | ||
| // - component unmounts; | ||
| useEffect(() => { | ||
| if (!exited) return; | ||
|
|
||
| handleHide(); | ||
| }, [exited, handleHide]); | ||
|
|
||
| useWillUnmount(() => { | ||
| handleHide(); | ||
| }); |
There was a problem hiding this comment.
is there no good way to write this as a single effect? e.g. calculate a shown value, and then have an effect block?
if (show && container && !foo) {
setFoo(true);
} else if (exited) {
setFoo(false);
}
useEffect(() => {
if (!foo) {
return undefined;
}
handleShow();
return handleHide;
}, [foo, handleShow, handleHide]);There was a problem hiding this comment.
ah maybe, wasn't sure if it helped us much tho. I guess it would simplify it nicely. I can try
There was a problem hiding this comment.
imma try that as a follow up
Co-Authored-By: Steven Loria <[email protected]>
Co-Authored-By: Jimmy Jia <[email protected]> Co-Authored-By: Steven Loria <[email protected]>
Co-Authored-By: Jimmy Jia <[email protected]>
| @@ -30,11 +30,11 @@ | |||
| "scripts": { | |||
| "bootstrap": "yarn && yarn --cwd www", | |||
| "build": "rimraf lib && yarn build:cjs && yarn build:esm && yarn build:pick", | |||
| "build:cjs": "babel src -d lib/cjs && rollup src/popper.js --file lib/cjs/popper.js --format cjs --name popper --plugin @rollup/plugin-node-resolve", | |||
| "build:cjs": "babel src -d lib/cjs && rollup src/popper.ts --file lib/cjs/popper.js --format cjs --name popper --plugin @rollup/plugin-node-resolve", | |||
The `react-overlays` switched to fully blown TS version as of 3.1.0: react-bootstrap/react-overlays#794 Should fix issue when installing current `react-overlays` with previous version of types. /cc @NicoleRauch Thanks! Fixes: DefinitelyTyped#40940
…zejewicz The `react-overlays` switched to fully blown TS version as of 3.1.0: react-bootstrap/react-overlays#794 Should fix issue when installing current `react-overlays` with previous version of types. /cc @NicoleRauch Thanks! Fixes: #40940
…ge by @peterblazejewicz The `react-overlays` switched to fully blown TS version as of 3.1.0: react-bootstrap/react-overlays#794 Should fix issue when installing current `react-overlays` with previous version of types. /cc @NicoleRauch Thanks! Fixes: DefinitelyTyped#40940
Started as a quick docs refactor but i figured I might as well switch to TS while i'm at it