feat: implement InternalOverlay component
#1581
Conversation
FeaturesBug Fixes
Tests
Contributors |
| import RenderIf from '../RenderIf'; | ||
| import useOutsideClick from '../../libs/hooks/useOutsideClick'; | ||
| import useWindowResize from '../../libs/hooks/useWindowResize'; |
There was a problem hiding this comment.
@LeandroTorresSicilia I have doubts we put to use the hooks and internal components in the examples?
There was a problem hiding this comment.
I think we should start documenting every internal implementation as part of the internal session, I will avoid code duplication since the library is huge and something we can end up repeating same implementation.
things under internal session will only for internal use and we specific to consumers of the library that the API of internal could change and it won't be considered breaking change
| variant="neutral" | ||
| icon={<PlusIcon />} | ||
| icon={<StyledPlusIcon />} | ||
| ref={ref} | ||
| onClick={handleClick} | ||
| /> | ||
| <InternalOverlay |
There was a problem hiding this comment.
@LeandroTorresSicilia here I also have doubts about how to export and import this component. It is not clear how InternalOverlay was imported
There was a problem hiding this comment.
relative-path since it's internal, it won't be part of the public API
src/libs/hooks/useWindowResize.js
Outdated
| }; | ||
| }, [isListening, listener]); | ||
|
|
||
| return [() => setIsListening(true), () => setIsListening(false)]; |
There was a problem hiding this comment.
With these 2 functions you also have to make it useCallback because otherwise they will be defined over and over again. If outside the hook other hook depends on them, you will cause that hook to be executed.
Every time you go to make a reusable hook everything you return must be done useMemo or useCallback
There was a problem hiding this comment.
@yvmunayev those 2 functions does not have dependencies, so useCallback will do nothing, I need some advice on this...
There was a problem hiding this comment.
These functions can be dependencies of other hooks, if you don't Memo it then the other hooks will always be executed. The conditional of the hooks uses the javascript comparison, where 2 functions, even if they are the same if the references are different, returns false.
src/libs/hooks/useWindowResize.js
Outdated
| @@ -0,0 +1,27 @@ | |||
| import { useEffect, useState, useCallback } from 'react'; | |||
|
|
|||
| export default function useWindowResize(handler) { | |||
There was a problem hiding this comment.
it should be something similar to useEffect, the implementation depend of when should listen(generally when something is open).
Possible API could be
useWindowResize(handler, isListening = true);
so it can be used to listen always or on and off
always:
useWindowResize(() => something()};
on and off
const [isOpen, setOpenState] = useState(false);
useWindowResize(() => setOpenState(false), isOpen);
| render: ComponentClass | FunctionComponent; | ||
| isVisible?: boolean; | ||
| triggerElementRef: RefObject | TriggerElementRefFunction; | ||
| positionResolver: (opts: PositionResolverOpts) => Position; |
There was a problem hiding this comment.
all props should be optional
|
@HellWolf93 there are a couple of tests failing |
LeandroTorresSicilia
requested review from
LeandroTorresSicilia
and removed request for
LeandroTorresSicilia
…react-rainbow into feat-InternalOverlay
fix: #1540
Changes proposed in this PR:
Add useWindowResize hook
Define InternalOverlay component typescript interface
Add tests for InternalOverlay component
Improve examples of InternalOverlay component
Fix triggerElementRef prop type
I have followed (at least) the PR section of the contributing guide.