Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added GridKeyboardNavigationContext #462

Merged
merged 22 commits into from
Jan 20, 2022

Conversation

laviomri
Copy link
Contributor

Task
Added a context which allows navigation between multiple navigable sections.
A GIF is worth a thousand words :)
navigation_context

In some cases where the navigated components are not in the same dimensions, the "jump" between the two components may look clanky (as seen in the gif). It's very difficult to fix, but probably possible.

Basic

  • Used plop (npm run plop) to create a new component.
  • PR has description.
  • New component is functional and uses Hooks.
  • Component defines PropTypes.

Style

  • Styles are added to NewComponent.modules.scss file inside of the NewComponent folder. Irrelevant
  • Component uses CSS Modules. Irrelevant
  • Design is compatible with Monday Design System.

Storybook

  • Stories were added to /src/NewComponent/__stories__/NewComponent.stories.js file.
  • Stories include all flows of using the component.

Tests

return (
<GridKeyboardNavigationContext.Provider value={keyboardContext}>
<Flex ref={wrapperRef}>
<DummyNavigableGrid ref={leftElRef} itemsCount={15} numberOfItemsInLine={4} itemPrefix="L "/>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The story doesn't look too good, but I wanted to emphasize the layout structure so the behavior will be clear. (you can check the gif in the PR's description).
Do you think I should change it?

Comment on lines 5 to 8
[NAV_DIRECTIONS.RIGHT]: new Map(),
[NAV_DIRECTIONS.LEFT]: new Map(),
[NAV_DIRECTIONS.UP]: new Map(),
[NAV_DIRECTIONS.DOWN]: new Map()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simple {} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since I use Objects (refs) as the keys. If I'll use a regular object as a map, the actual value which will be used as key is the result of .toString():
image

on the contrary, a Map can handle objects as keys

…om:mondaycom/monday-ui-react-core into feature/omri/use-grid-keyboard-navigation-context
…to feature/omri/user-grid-keyboard-navigation
…om:mondaycom/monday-ui-react-core into feature/omri/use-grid-keyboard-navigation-context
Base automatically changed from feature/omri/user-grid-keyboard-navigation to master January 20, 2022 12:55
@laviomri laviomri merged commit f69112a into master Jan 20, 2022
@laviomri laviomri deleted the feature/omri/use-grid-keyboard-navigation-context branch January 20, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants