Menu grid item #506
Menu grid item #506
Conversation
…nnecessary rerenders
| @@ -91,12 +95,10 @@ const Menu = forwardRef( | |||
| setIsInitialSelectedState(true); | |||
| }, [setIsInitialSelectedState]); | |||
|
|
|||
| useLayoutEffect(() => { | |||
| useEffect(() => { | |||
There was a problem hiding this comment.
When using requestAnimationFrame, long-pressing the arrows didn't work properly with the focus/blur handlers.
There was a problem hiding this comment.
@laviomri it looks good, but lets try to think if it is a breaking change?
There was a problem hiding this comment.
theoretically, it may cause a very very short delay until the focus is actually called (since useEffect is async and useLayoutEffect is sync, basically).
In practice I've played with it quite a bit and didn't see any issue with it.. Do you have specific concerns?
…to feature/omri/menu-enhancements
…/monday-ui-react-core into feature/omri/menu-grid-item
…to feature/omri/menu-grid-item
…to feature/omri/menu-grid-item
| @@ -91,12 +95,10 @@ const Menu = forwardRef( | |||
| setIsInitialSelectedState(true); | |||
| }, [setIsInitialSelectedState]); | |||
|
|
|||
| useLayoutEffect(() => { | |||
| useEffect(() => { | |||
There was a problem hiding this comment.
@laviomri it looks good, but lets try to think if it is a breaking change?
| @@ -0,0 +1,126 @@ | |||
| import { renderHook } from "@testing-library/react-hooks"; | |||
There was a problem hiding this comment.
b e a u t i f u l l !!!
|
|
||
| export const useFocusGridItemByActiveStatus = ({ wrapperRef, childRef, index, activeItemIndex }) => { | ||
| const { lastNavigationDirectionRef } = useLastNavigationDirection(); | ||
| const isActive = useMemo(() => index === activeItemIndex, [activeItemIndex, index]); |
There was a problem hiding this comment.
maybe we can move it into the use effect?
There was a problem hiding this comment.
@orrgottlieb this is intentional - the calculations of isActive may run a few times without changing the result (e.g. when index = 2 and the activeItemIndex changes from 3 to 4, and then to 0. isActive will remain false all the time). I want the content of the useEffect to run ONLY when the isActive actually changes, not when the calculation is performed.
| onOutboundNavigation: (elementRef, direction) => { | ||
| innerKeyboardContext.onOutboundNavigation(elementRef, direction); | ||
|
|
||
| switch (direction) { |
There was a problem hiding this comment.
no right because of sub menus ? - i guess we don't allow sub menu and a MenuItemGrid
There was a problem hiding this comment.
@orrgottlieb if we would have supported submenus inside a MenuGridItem, then we should have added a handler for outbound right navigation... however, I don't think such behavior makes any sense (I mean, it will be a grid that opens a sub menu when navigating to the right of it. Sounds odd, no? 🤔 ).
In any case, if we'll migrate the Menu implementation to use the grid keyboard navigation, it will be easier to support such (odd) behaviors 😄
| @@ -75,3 +75,4 @@ export { default as ColorUtils } from "../utils/colors-utils"; | |||
| export { default as Slider } from "./Slider/Slider"; | |||
| export { default as IconButton } from "./IconButton/IconButton"; | |||
| export { default as Flex } from "./Flex/Flex"; | |||
| export { default as MenuGridItem } from "./Menu/MenuGridItem/MenuGridItem"; | |||
There was a problem hiding this comment.
please add to publish components as well
There was a problem hiding this comment.
@orrgottlieb I'm not sure I understand the usage of each file correctly..
Also, should I add useGridKeyboardNavigation to the end of published-components as well? (EDIT: I've added it for now)
Added
MenuGridItemBasic
npm run plop) to create a new component.PropTypes.Style
NewComponent.modules.scssfile inside of theNewComponentfolder.Storybook
/src/NewComponent/__stories__/NewComponent.stories.jsfile.Tests